-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use BufferedRead when reading #56
base: main
Are you sure you want to change the base?
Conversation
I can now see that the small reads are actually not happening, so maybe the motivation to include this is not as strong as I first thought. |
src/reader.rs
Outdated
// The matches/if let dance is to fix lifetime of the borrowed inner connection. | ||
#[cfg(feature = "embedded-tls")] | ||
if matches!(self.buffered.bypass(), Ok(HttpConnection::Tls(_))) { | ||
if let HttpConnection::Tls(ref mut tls) = self.buffered.bypass().unwrap() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bypass
is a neat idea!
So do I understand correctly that if let Ok(HttpConnection::Tls(ref mut tls)) = self.buffered.bypass() {
without the outer matches!
would not work˙, even if you wrap the whole block in a new scope?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Year, I have tried multiple versions, which all fail to compile. The problem is that self.buffered
is still borrowed in the else branch, and I cannot figure out how to avoid that. Any good proposals?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to change .bypass()
to return Result<&mut T, &mut Self>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you think that would make it work, then yes:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I can try locally first :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, no, it can't because we still need to handle the HttpConnection::Plain
branch :(
Yeah the network stack won't receive bytes one by one but this is certainly a cleaner implementation than what we had before. Minus the |
Currently when e.g. reading a chunked response body, we request a
read()
with a single byte buffer. This PR uses theBufferedRead
as an intermediate to greatly avoid small connection reads when tls is not in use.cc @bugadani