Fix connection recycling with stalled HTTP 1.1 streams#561
Conversation
…to determine whether we need to close a connection when a user closes it
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #561 +/- ##
==========================================
- Coverage 79.79% 79.63% -0.16%
==========================================
Files 28 28
Lines 12085 12104 +19
==========================================
- Hits 9643 9639 -4
- Misses 2442 2465 +23 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
TingDaoK
left a comment
There was a problem hiding this comment.
Overall, I am not against the idea when user releases the connection back to the pool, close it when there is still streams alive or even error it out.
If there is outstanding request/response and user release it back to the pool, it to me like freeing an object that is still actively in use. We can be proactive to either prevent it, or be nice to close the connection so that the requests will be closed as well.
…t as it should fail once we fix the multiple streams on single connection thing
…sting as well as checking the connection for idle streams
| static bool s_h2_connection_is_idle(const struct aws_http_connection *connection_base) { | ||
| /* HTTP/2 multiplexes streams and handles cleanup via GOAWAY. | ||
| * The connection manager uses separate H2-specific mechanisms for lifecycle management. */ | ||
| (void)connection_base; | ||
| return true; | ||
| } |
There was a problem hiding this comment.
i don't know what this means...
I'd say either we raise error that it's not currently supported in http/2 or actually support this helper (not for the reason to control the lifetime from connection manager, but it is nice to have to check if the connection is idle or not)
There was a problem hiding this comment.
HTTP/2 doesn't run into the issue because it multiplexes streams (preventing a stream not completed from blocking the connection entirely) and uses GOAWAY (
aws-c-http/include/aws/http/connection.h
Lines 79 to 93 in 8acf604
new_request_allowed() from returning true. It also uses the stream manager to check if it has streams assigned to it before allowing it to be released. Re-reading this description, it's pretty weird without context...
This function being added to the v-table was a result of needing to get this info from a testing perspective in our test framework. That meant having to add it to HTTP/2 and figured it'd be easiest to just make it a no-op.
It makes sense to allow it to return the actual idle state of an HTTP/2 connection instead of just returning true since it's here. I've added a best-effort check on the idle state but removed this from the being checked on HTTP/2 connections during the recycle check because HTTP/2 connections have different behavior that doesn't require it.
The
HttpClientConnectionManagerusingmanual_window_management = truewas able to release a connection back to the pool while a HTTP 1.1 stream was still in progress. e.g. the stream window filled to zero and the user never calledupdate_window. The connection manager would be able to vend the same connection for a new request but because HTTP 1.1 processes streams in FIFO order, the new stream's response callback would never fire and be permanently blocked waiting for the stalled stream to complete. This caused requests to hang indefinitely after a previous request on the same connection was abandoned with an unconsumed response body.This can be seen as a user error but we should handle it.
A num_streams_in_progress counter is added to
aws_h1_connection.synced_datato track client streams that have been submitted but not yet completed. The counter is incremented when a stream is activated and decremented in the existing critical section ofs_stream_complete()An internal
aws_http_connection_is_connection_idle()function is introduced, backed by a connection vtable function and wired through the connection manager's system vtable. For H1 connections, this returns false when num_streams_in_progress > 0. For H2 connections, it unconditionally returns true (H2 multiplexes streams independently and uses GOAWAY for lifecycle management).The connection manager release path now checks both
new_requests_allowed(connection alive/not errored) andis_connection_idle(no orphaned streams). If the connection is not idle, it is destroyed instead of recycled, preventing the stall.Tests:
Check that new requests are allowed during pipelining.
Check that
is_connection_idlereturns false during active/stalled streams and true after completion.CI:
split out the downstream ci job into its own workflow to allow us to retry the significantly shorter running jobs if they failed a flaky test.
fail-fast on matrix CI jobs to false so we can allow other jobs to continue running if one flaky test fails in one matrix element.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.