Skip to content

httpclient: use a 16kb read buffer for macOS#5432

Merged
pks-t merged 1 commit intomasterfrom
ethomson/sslread
Mar 5, 2020
Merged

httpclient: use a 16kb read buffer for macOS#5432
pks-t merged 1 commit intomasterfrom
ethomson/sslread

Conversation

@ethomson
Copy link
Member

@ethomson ethomson commented Mar 1, 2020

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).

@ethomson ethomson added the 1.0 label Mar 1, 2020
@ethomson ethomson linked an issue Mar 1, 2020 that may be closed by this pull request
@ethomson
Copy link
Member Author

ethomson commented Mar 1, 2020

@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.

@implausible
Copy link
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

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).
Copy link
Member

@pks-t pks-t left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

@pks-t pks-t merged commit 76e4596 into master Mar 5, 2020
@ethomson ethomson deleted the ethomson/sslread branch June 3, 2020 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0.99.0: Performance regression in Mac OS clone

3 participants