Drain the accepted streams after a graceful GOAWAY - #31
Merged
samuel-williams-shopify merged 6 commits intoAug 31, 2026
Conversation
Connection#receive_goaway closed the connection immediately, whatever the error code. For a graceful GOAWAY (error code 0) that is wrong: the peer will not accept new streams, but it is still processing the streams at or below last_stream_id and will send their responses (RFC 9113 6.8). Closing at once makes those requests fail with "Connection closed with N active stream(s)!" even though the peer processed them, which for a non-idempotent request cannot be retried. On a graceful GOAWAY the connection now records that it is going away, refuses and removes the streams above last_stream_id as before, and closes when the last accepted stream completes. A non-zero error code is unchanged. The state of the connection is decided before any stream's closed hook runs, so a hook which raises cannot leave it undecided and one which creates a stream cannot defeat the "nothing left to drain" check. create_stream refuses to open a locally-initiated stream once a GOAWAY has been received; streams the peer initiates are not covered by last_stream_id and stay legal. Connection#close detaches the active streams before closing them, so a re-entrant close cannot report a fabricated EOFError in place of the real error. Adds Connection#goaway_received? and Connection#draining?, which a client needs in order to stop offering the connection to new requests while it drains.
Merged
3 tasks
Assisted-By: devx/618580b0-d55f-4c2e-95b6-87e648f60543
Assisted-By: devx/618580b0-d55f-4c2e-95b6-87e648f60543
Contributor
|
Removed the generic Connection#close stream-detachment change from this PR. It was intended to make close callbacks re-entrant-safe (and would make draining? false before callbacks), but it is not required for graceful GOAWAY handling and changes shutdown behavior outside this feature. It also had no focused regression coverage and could leave later detached streams unclosed if one callback raised. Verified with the original close behavior: protocol-http2 239/239 tests (1,194 assertions), async-http HTTP/2 17/17 tests (49 assertions). Any re-entrant close work should be handled separately with dedicated tests. |
Assisted-By: devx/618580b0-d55f-4c2e-95b6-87e648f60543
Assisted-By: devx/618580b0-d55f-4c2e-95b6-87e648f60543
Assisted-By: devx/618580b0-d55f-4c2e-95b6-87e648f60543
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.
What this fixes
Connection#receive_goawaycloses the connection immediately, whatever the error code. For agraceful
GOAWAY(error code0) that is wrong: the peer is telling us it will not accept newstreams, but it is still processing the streams at or below
last_stream_idand will sendtheir responses (RFC 9113 §6.8).
Because
close!runs straight away,closed?is true, the caller stops reading, andConnection#closethen raisesEOFError: Connection closed with N active stream(s)!intoexactly those streams — requests the peer has already processed, whose side effects have
already happened, and which cannot be retried if they are not idempotent.
nginx does this on the
keepalive_requests-th request of every HTTP/2 connection (default1000), onkeepalive_time(default1h), and on every reload, so a client with requests inflight loses a burst of them each time.
The change
On
GOAWAYwith error code0:GOAWAYwas received instead of closing the connection;last_stream_idwithProtocol::HTTP::RefusedError(unchanged) andremove them from the connection, so what remains is exactly the set of streams we are waiting
for;
none.
A non-zero error code still closes the connection immediately and raises
GoawayError, asbefore.
The state of the connection is decided before any stream's
closedhook runs, so that a hookwhich raises cannot leave the connection in an undecided state, and one which creates a stream
cannot defeat the "nothing left to drain" check.
Connection#create_streamnow refuses to open a locally-initiated stream once aGOAWAYhasbeen received, since RFC 9113 §6.8 says receivers of a
GOAWAYmust not open additionalstreams. Streams the peer initiates — a server pushing on a stream it already accepted — are
not covered by
last_stream_idand remain legal, so they are still accepted.Adds
Connection#goaway_received?, which a client uses to stop offering the connection to newrequests once a
GOAWAYhas been received. The remaining lifecycle is expressed by theconcrete
closed?state and the registered streams; no derived draining predicate is exposed.Behaviour change
closed?is no longer true immediately after receiving a gracefulGOAWAY— it becomes truewhen the last accepted stream completes. Anything that gated new requests on
!closed?shouldgate on
!goaway_received?instead;create_streamnow enforces this for locally-initiatedstreams.
The existing "client can handle graceful shutdown" test is updated to assert the new lifecycle,
and there are new tests for draining several accepted streams, for the case where there is
nothing to drain, for the
create_streamrestriction, for push promises arriving during adrain, and for a stream callback which raises.
Details
The full write-up, a reproduction against real nginx, and the client-side half of the fix (pool
reuse and connection close) are in socketry/async-http#245. This change alone is not enough:
without it the connection is closed under the in-flight streams, and without the
async-httpside the pool keeps handing the draining connection to new requests.
Types of Changes
Contribution