httpclient: use a 16kb read buffer for macOS#5432
Merged
Conversation
Member
Author
|
@implausible can you test this? You're seeing different behavior than I am, presumably due to macOS versions. But I think that this is the root of the problem. |
Contributor
|
@ethomson I don't have a Mac at home, but I put that github action suite to use and can confirm this seems to solve the issue! Awesome sleuthing. https://github.com/implausible/libgit2/runs/478343030?check_suite_focus=true |
5 tasks
Use a 16kb read buffer for compatibility with macOS SecureTransport. SecureTransport `SSLRead` has the following behavior: 1. It will return _at most_ one TLS packet's worth of data, and 2. It will try to give you as much data as you asked for This means that if you call `SSLRead` with a buffer size that is smaller than what _it_ reads (in other words, the maximum size of a TLS packet), then it will buffer that data for subsequent calls. However, it will also attempt to give you as much data as you requested in your SSLRead call. This means that it will guarantee a network read in the event that it has buffered data. Consider our 8kb buffer and a server sending us 12kb of data on an HTTP Keep-Alive session. Our first `SSLRead` will read the TLS packet off the network. It will return us the 8kb that we requested and buffer the remaining 4kb. Our second `SSLRead` call will see the 4kb that's buffered and decide that it could give us an additional 4kb. So it will do a network read. But there's nothing left to read; that was the end of the data. The HTTP server is waiting for us to provide a new request. The server will eventually time out, our `read` system call will return, `SSLRead` can return back to us and we can make progress. While technically correct, this is wildly ineffecient. (Thanks, Tim Apple!) Moving us to use an internal buffer that is the maximum size of a TLS packet (16kb) ensures that `SSLRead` will never buffer and it will always return everything that it read (albeit decrypted).
725b5f9 to
502e5d5
Compare
pks-t
approved these changes
Mar 5, 2020
Member
pks-t
left a comment
There was a problem hiding this comment.
Not saying that this looks obviously correct to me, but the explanation is reasonable and @implausible confirmed it fixes the issue ;) Thanks a lot to both of you!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Use a 16kb read buffer for compatibility with macOS SecureTransport.
SecureTransport
SSLReadhas the following behavior:This means that if you call
SSLReadwith a buffer size that is smaller than what it reads (in other words, the maximum size of a TLS packet), then it will buffer that data for subsequent calls. However, it will also attempt to give you as much data as you requested in your SSLRead call. This means that it will guarantee a network read in the event that it has buffered data.Consider our 8kb buffer and a server sending us 12kb of data on an HTTP Keep-Alive session. Our first
SSLReadwill read the TLS packet off the network. It will return us the 8kb that we requested and buffer the remaining 4kb. Our secondSSLReadcall will see the 4kb that's buffered and decide that it could give us an additional 4kb. So it will do a network read.But there's nothing left to read; that was the end of the data. The HTTP server is waiting for us to provide a new request. The server will eventually time out, our
readsystem call will return,SSLReadcan return back to us and we can make progress.While technically correct, this is wildly ineffecient. (Thanks, Tim Apple!)
Moving us to use an internal buffer that is the maximum size of a TLS packet (16kb) ensures that
SSLReadwill never buffer and it will always return everything that it read (albeit decrypted).