Repository navigation
Drain open streams after receiving a graceful GOAWAY - #1346
Open
benpbolton wants to merge 2 commits into
Open
benpbolton wants to merge 2 commits into
benpbolton wants to merge 2 commits into
Conversation
RFC 9113, Section 6.8 lets streams at or below a GOAWAY's last stream ID complete. Servers often shut down in two stages: GOAWAY(NO_ERROR) with a high last stream ID, a PING, then a GOAWAY with the real last stream ID. h2 moved to CLOSED on any GOAWAY, so the PING or the rest of an open response raised "Invalid input ... in state ConnectionState.CLOSED". After a GOAWAY with NO_ERROR, the connection now drains: - The state stays CLOSED, so consumers that check for CLOSED still stop opening streams on the connection. H2ConnectionStateMachine.draining reports the drain. - Frames for open streams are still processed. PING and SETTINGS are still acknowledged. PUSH_PROMISE can still be received. The outbound buffer is kept. - The drain keeps a shadow of the open state, so the client and server rules of the open connection still apply. - New local streams (send_headers on a new stream ID, push_stream, initiate_upgrade_connection) raise ProtocolError before any state change, and the drain continues. - A further graceful GOAWAY continues the drain. A GOAWAY with an error code, an invalid input or close_connection() ends it. A graceful GOAWAY never reopens a connection that is already closed. ConnectionTerminated is still fired for every GOAWAY. h2 does not reset or filter streams above last_stream_id; the application decides. Fixes python-hyper#1199. Fixes the receive side of python-hyper#1181. Builds on python-hyper#1324. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.
Fixes #1199.
This covers the receive side of #1181. Sending-side draining is still separate work. The companion mitmproxy change is mitmproxy/mitmproxy#8580.
When a peer sends
GOAWAY(NO_ERROR), h2 currently entersCLOSEDand rejects subsequent frames. That can interrupt open requests when the peer sends a PING or the rest of a response. RFC 9113 §6.8 allows those streams to finish.This adds a drain after a graceful GOAWAY. Existing streams keep working, the outbound buffer is preserved, and PING and SETTINGS are still acknowledged. Opening a new local stream raises
ProtocolErrorwithout interrupting the drain. The usual client/server rules still apply, and the peer can still open streams, including through PUSH_PROMISE.An error GOAWAY, connection error, or
close_connection()ends the drain. A later graceful GOAWAY won't reopen a closed connection. After terminal closure, leftover frames other than GOAWAY are ignored.The public state stays
CLOSEDto preserve httpcore's connection-reuse check.H2Connection.state_machine.drainingtells callers whether existing streams can continue.ConnectionTerminatedstill fires for every GOAWAY, soNO_ERRORalone isn't enough to tell whether the connection is draining. httpcore and httpx still need their own changes to finish requests after GOAWAY.Applications remain responsible for streams above
last_stream_id: they're safe to retry elsewhere, but h2 doesn't reset or filter them. It also doesn't enforce a non-increasinglast_stream_idacross GOAWAY frames.The full suite passes: 1,706 tests, with 100% line and branch coverage in
connection.py. Ruff,mypy --strict-bytes, and the docs build with-Wpass. In a live reproduction against Google, h2 4.4.1 fails onRECV_PINGafter 240 seconds idle; this branch acknowledges the PING between the two GOAWAY frames.This builds on @adarshx01's #1324, included with the original commit and authorship. The follow-up keeps draining connections out of its ignore guard and changes its test to use an error GOAWAY. Thanks also to @zanieb, @dimaqq, and @neoLsH for the investigation and design discussion in #1181.
@neoLsH, I'd welcome your review and would be happy to add co-authorship if we incorporate your work. @varjoranta, your offer to test against GitHub's connection turnover would be useful here too.