http: don't close TCP connection when response body is empty and request is retried#5037
Conversation
|
Thanks, at a high level this LGTM. Can you check CI? |
9ad4265 to
d4c1f6e
Compare
|
The CI issue was that the code was marking the stream as ended for |
0441906 to
05c0d9d
Compare
mattklein123
left a comment
There was a problem hiding this comment.
Thanks this LGTM. @alyssawilk can you take a quick look?
There was a problem hiding this comment.
We already have a test that covers 1xx, but can you add a test for maybe 200 with content_length zero also?
alyssawilk
left a comment
There was a problem hiding this comment.
Code LGTM, thanks for the fix!
I'd say it's worth a release note though even though it's not technically required. It may not change user-facing behavior but it will have a non-trivial (positive!) effect for folks running H1 upstreams and if anyone is doing whacky non-spec compliant responses it'd be good to give them a heads up.
|
SGTM re: release note. @julia-stripe do you mind adding one? |
|
added a note! |
alyssawilk
left a comment
There was a problem hiding this comment.
Did that update get pushed upstream? I'm not seeing a change yet :-)
Signed-off-by: Julia Evans <julia@stripe.com>
Signed-off-by: Julia Evans <julia@stripe.com>
Signed-off-by: Julia Evans <julia@stripe.com>
Signed-off-by: Julia Evans <julia@stripe.com>
Signed-off-by: Julia Evans <julia@stripe.com>
Signed-off-by: Julia Evans <julia@stripe.com>
Signed-off-by: Julia Evans <julia@stripe.com>
f72c3b4 to
047aba3
Compare
|
rebased because it said there was a merge conflict |
alyssawilk
left a comment
There was a problem hiding this comment.
Haha - you caught me mid "but you'll have to pull from master" comment :-)
| * http: augmented the `sendLocalReply` filter API to accept an optional `GrpcStatus` | ||
| value to override the default HTTP to gRPC status mapping. | ||
| * load balancer: added a `configuration <envoy_api_msg_Cluster.LeastRequestLbConfig>` option to specify the number of choices made in P2C. | ||
| * http: no longer close the TCP connection when a HTTP/1 request is retried due |
There was a problem hiding this comment.
nit: sorry, can you alpha order this
…est is retried (envoyproxy#5037) Signed-off-by: Julia Evans <julia@stripe.com> Signed-off-by: Fred Douglas <fredlas@google.com>
Description:
This fixes the issue described in #5023: right now we set
end_stream=falsefor many HTTP/1 responses even if the HTTP body is empty (and the stream has ended). Retried requests withend_stream=falseclose the connection, so this results in the TCP connection being closed in many cases where it could be kept open, which results in unnecessary connection churn.With this change, when an upstream returns a HTTP/1 response with an empty body and the request is retried, the TCP connection will no longer be closed.
Risk Level:
Low
Testing:
Did manual testing to see that the connection is no longer closed when an upstream returns a 503, and added a unit test to ensure that
end_streamis set correctly.Release note
When a HTTP/1 request is retried due to a 5xx/499 response, we now keep the TCP connection open instead of closing it
Fixes #5023