transport: Discard the buffer when empty after http connect handshake#7424
transport: Discard the buffer when empty after http connect handshake#7424arjan-bal merged 4 commits intogrpc:masterfrom
Conversation
arjan-bal
added
the
Type: Performance
arjan-bal
force-pushed
the
http-connect-buffer
branch
from
57074b6 to
b0e2b5d
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7424 +/- ##
========================================
Coverage 81.50% 81.50%
========================================
Files 350 354 +4
Lines 26877 27078 +201
========================================
+ Hits 21906 22071 +165
- Misses 3776 3813 +37
+ Partials 1195 1194 -1
|
internal/transport/proxy_test.go
Outdated
| c.SetReadDeadline(time.Now().Add(20 * time.Millisecond)) | ||
|
|
||
| gotServerMessage := make([]byte, len(serverMessage)) | ||
| // This call will return a deadline exceeded error if the server has nothing | ||
| // to send. This is expected. | ||
| if _, err := c.Read(gotServerMessage); err != nil { | ||
| t.Logf("Got error while reading message from server: %v", err) | ||
| } |
There was a problem hiding this comment.
Similar; I don't really like adding a magic deadline to things that we know will timeout sometimes.
Probably we should only do this read if len(serverMessage) != 0, and then we can apply a longer deadline while not impacting code that doesn't use it?
There was a problem hiding this comment.
Changed, now the proxy is configured to wait for server hello.
internal/transport/proxy_test.go
Outdated
| } | ||
|
|
||
| func testHTTPConnect(t *testing.T, proxyURLModify func(*url.URL) *url.URL, proxyReqCheck func(*http.Request) error) { | ||
| func testHTTPConnect(t *testing.T, proxyURLModify func(*url.URL) *url.URL, proxyReqCheck func(*http.Request) error, serverMessage []byte) { |
There was a problem hiding this comment.
We probably should add a struct about now to hold all the parameters (unless we decide to test this code differently per the above comment).
internal/transport/proxy_test.go
Outdated
| } | ||
|
|
||
| if len(args.serverMessage) > 0 { | ||
| c.SetReadDeadline(time.Now().Add(defaultTestTimeout)) |
There was a problem hiding this comment.
There's now 3 places where we're setting this deadline.
Maybe instead when the conn is created in proxyDial we should just do c.SetDeadline(....defaultTestTimeout) (note Deadline not Read to also abort writes)?
There was a problem hiding this comment.
During testing, I noticed that the test can hang in proxyDial if the proxy server waits for a server hello when the server doesn't intend to send one. This was due to a logical error on my part which I fixed.
To be safe, I added read deadlines in both the places where reads could hang.
Maybe instead when the conn is created in proxyDial we should just do c.SetDeadline(....defaultTestTimeout) (note Deadline not Read to also abort writes)?
Do you mean adding a deadline in the actual proxyDial() implementation by introducing a timeout that only the tests use?
Or are you referring to adding a deadline on the conn returned by proxyDial in the test code?
The latter would not prevent proxyDial from hanging when the proxy server is stuck reading the server hello. The former would work but it involves changing non-test code, which I was trying to avoid.
There was a problem hiding this comment.
Sorry, I didn't mean in proxyDial... I meant the connection it returns, we could immediately, always, do a SetDeadline(...), instead of doing it later and conditionally.
There was a problem hiding this comment.
Changed SetReadDeadline to SetDeadline. Moved the SetDeadline immediately after the proxyDial call. The SetDeadline in proxyServer.run() is still present to avoid proxyDial from hanging indefinitely in case of test failures.
…grpc#7424) * Discard the buffer when empty after http connect handshake * configure the proxy to wait for server hello * Extract test args to a struct * Change deadline sets
2 participants
Fixes: #7327
The buffer used to read the http connect response could contain extra bytes from the target server, so we can't discard it. However, in many cases where the server waits for the client to send the first message (e.g. when TLS is being used), the buffer will be empty, so we can avoid the overhead of reading through this buffer.
This PR also adds a unit test to ensure the buffer is not discarded when it has unread data.
RELEASE NOTES: