http2: distinguish graceful peer shutdown from forceful termination#1917
http2: distinguish graceful peer shutdown from forceful termination#1917aki1770-del wants to merge 2 commits into
Conversation
…art-lang#1913) When a peer sends GOAWAY with NO_ERROR and then closes the underlying transport, Connection._terminate() is invoked with causedByTransportError=true and previously surfaced TransportConnectionException("Connection is being forcefully terminated.") to all sub-components — the same exception text used for genuine forceful-termination paths. Consumers (notably grpc-dart, see the referenced grpc/grpc-dart#827 attempt) could not distinguish a clean peer-initiated shutdown from an actual fault without inspecting the exception text. This change detects the graceful-shutdown path in _terminate() (peer has set FinishingPassive via processGoawayFrame + _finishing(false)) and surfaces a distinct exception under that condition: TransportConnectionException( errorCode: ErrorCode.NO_ERROR, message: 'Connection gracefully closed by peer.', ) Forceful-termination paths continue to surface the prior message ("Connection is being forcefully terminated.") unchanged. Consumers can distinguish either via .errorCode == NO_ERROR or the new message text. Per the issue thread, brianquinlan voted for shape (b) — breaking but cleaner. This implementation realizes that shape while keeping the TransportConnectionException type stable so sub-component onTerminated handlers (e.g. SettingsHandler null-check on error) keep working without further refactoring. Adds a regression test under transport-test verifying the graceful-close path surfaces NO_ERROR + does not contain the forceful- termination text. Closes dart-lang#1913 (pending maintainer review of the chosen shape).
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
CLA signed. Ready for review when convenient. |
|
@mosuem do you want to look at this or should I? |
|
Thanks @brianquinlan — no rush on my end, and I appreciate you picking it up. Happy to help however's easiest for you and @mosuem: I can walk through the graceful-peer-shutdown vs forceful-termination distinction, point you at the test coverage, or rebase if it's drifted at all. Whenever's convenient. |
|
Added a small suggestion in #1946 - @aki1770-del let me know what you think! |
|
Thanks @mosuem — this is nicer than what I had. Driving the graceful/forceful distinction off the actual connection state instead of the message text is the right call. Pulled #1946 and ran it locally — all 117 http2 tests pass (incl. your two new ones, Two things I like beyond the
Happy to take it. Do you want to merge #1946 into the PR branch, or shall I fold the change into #1917 with your co-authorship? Either way — thanks for shaping it. |
|
Please take it over into your PR. Thanks! |
|
Done — cherry-picked your commit onto this branch (kept your authorship on it), so #1917 now carries the connection-state approach. Ran the full http2 suite on the merged branch: 117/117 pass, analyze clean, including your |
PR HealthUnused Dependencies ✔️
For details on how to fix these, see dependency_validator. This check can be disabled by tagging the PR with Breaking changes ✔️
This check can be disabled by tagging the PR with API leaks ✔️The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
This check can be disabled by tagging the PR with Changelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. This check can be disabled by tagging the PR with Coverage ✔️
This check for test coverage is informational (issues shown here will not fail the PR). This check can be disabled by tagging the PR with License Headers ✔️
All source files should start with a license header. Unrelated files missing license headers
This check can be disabled by tagging the PR with |
Summary
Per #1913 and @brianquinlan's vote for shape (b) ("breaking but cleaner"), this PR distinguishes graceful peer-initiated shutdown (GOAWAY with
NO_ERRORfollowed by transport close) from forceful termination at theConnection._terminate()site.Behavior change
When the peer sends
GOAWAYwitherrorCode == NO_ERRORand then closes the transport,_terminate()now detects the passive-finishing state (set byprocessGoawayFrame + _finishing(active: false)) and surfaces:instead of the prior:
Forceful-termination paths (
onError,_frameWriter.doneFuture, settings handshake failure, etc.) continue to surface the prior"forcefully terminated"text unchanged.Why this shape vs literal "complete cleanly without exception"
A literal reading of issue-body shape (b) would have pending operations (e.g. an in-flight
connection.ping()or a settings ACK) complete cleanly, not error. That requires passingnullto sub-componentterminate()methods, which breaks several existingonTerminated(Object? error)impls that null-check the error (e.g.SettingsHandler.onTerminatedline 165 useserror!).The simplest implementation that delivers the shape-(b) intent — let callers distinguish graceful from forceful close — is to keep the existing
TransportConnectionExceptiontype but flag the graceful case viaerrorCode == NO_ERROR+ distinct message text. This minimizes sub-component refactoring while still being a breaking change at the consumer-text level. Open to amending toward the literal interpretation if you prefer the deeper refactor.Open questions for review
Exception text vs subtype: would you prefer a dedicated
GracefulShutdownException extends TransportConnectionExceptionsubtype rather than reusingTransportConnectionExceptionwith a newerrorCode/message? Subtype is more typesafe for caller-sideis-checks but slightly larger API surface.Sub-component refactoring: shape (b) literal — having sub-components'
donefutures complete cleanly on graceful close — would require updatingSettingsHandler,PingHandler,_closeStreamAbnormallyetc. to handle a null/graceful sentinel. Worth doing now (in this PR) or as a follow-up?mosuem: @brianquinlan tagged you on the issue; any thoughts on the chosen shape?
Test coverage
Adds
graceful-server-finish-distinct-shutdown-signalundertransport-testinpkgs/http2/test/transport_test.dart:ping()with server-sidefinish()TransportConnectionException(errorCode == NO_ERROR)+ does NOT contain"forcefully terminated"textAll existing
transport-testcases pass (15/15 including the new one).dart analyze: 0 issues.CHANGELOG
pkgs/http2/CHANGELOG.mdupdated under3.0.1-wipwith aBREAKINGentry noting the migration shape for consumers that matched the prior"forcefully terminated"text.Draft status
Opening as DRAFT to surface the implementation choice for @brianquinlan + @mosuem review before marking ready. Happy to amend toward shape (a) (
gracefulShutdown: boolfield on existing exception) or a literal shape (b) refactor depending on your preference.Closes #1913 (pending maintainer ratification).