From f3e7048ad44434cc94c972cba5ed85bfcf7614bd Mon Sep 17 00:00:00 2001 From: Akihiko Komada Date: Mon, 11 May 2026 08:33:12 +0900 Subject: [PATCH] http2: distinguish graceful peer shutdown from forceful termination (#1913) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #1913 (pending maintainer review of the chosen shape). --- pkgs/http2/CHANGELOG.md | 11 +++++++++ pkgs/http2/lib/src/connection.dart | 38 +++++++++++++++++++++++------ pkgs/http2/test/transport_test.dart | 37 ++++++++++++++++++++++++++++ 3 files changed, 79 insertions(+), 7 deletions(-) diff --git a/pkgs/http2/CHANGELOG.md b/pkgs/http2/CHANGELOG.md index b98deeeefa..a1e775ea7e 100644 --- a/pkgs/http2/CHANGELOG.md +++ b/pkgs/http2/CHANGELOG.md @@ -1,6 +1,17 @@ ## 3.0.1-wip - Gracefully handle receiving headers on a stream that the client has canceled. (#1799) +- **BREAKING (per #1913)**: distinguish graceful peer-initiated + shutdown (GOAWAY with `NO_ERROR` followed by transport close) from + forceful termination. Under the graceful path, pending operations + error with `TransportConnectionException(errorCode: NO_ERROR, + message: "Connection gracefully closed by peer.")` instead of the + prior conflated `"Connection is being forcefully terminated."` text. + Consumers that previously matched the forceful-termination text to + detect any peer-side shutdown should switch to observing + `TransportConnectionException.errorCode == ErrorCode.NO_ERROR` (or + the new message text) to distinguish graceful from forceful close. + (#1913) ## 3.0.0 diff --git a/pkgs/http2/lib/src/connection.dart b/pkgs/http2/lib/src/connection.dart index 6a9f3fc9de..d6d368fcea 100644 --- a/pkgs/http2/lib/src/connection.dart +++ b/pkgs/http2/lib/src/connection.dart @@ -453,6 +453,20 @@ abstract class Connection { }) { // TODO: When do we complete here? if (_state.state != ConnectionState.Terminated) { + // Detect graceful-shutdown path per issue #1913 shape (b): the peer + // has already sent a GOAWAY with NO_ERROR (transitioning us to + // passive-finishing state) and the transport then closed as part of + // that graceful flow. The transport-close that triggered this + // _terminate() call is expected, not an error condition — so we + // complete sub-components without an exception rather than surfacing + // `TransportConnectionException("Connection is being forcefully + // terminated.")`. BREAKING vs prior behavior where this path always + // raised the forceful-termination exception. + final isGracefulShutdown = + causedByTransportError && + _state.isFinishing && + (_state.finishingState & ConnectionState.FinishingPassive) != 0; + _state.state = ConnectionState.Terminated; var cancelFuture = Future.sync(_frameReaderSubscription.cancel); @@ -469,13 +483,23 @@ abstract class Connection { // We ignore any errors after writing to [GoawayFrame] }); - // Close all lower level handlers with an error message. - // (e.g. if there is a pending connection.ping(), it's returned - // Future will complete with this error). - var exception = TransportConnectionException( - errorCode, - 'Connection is being forcefully terminated.', - ); + // Close all lower level handlers. For graceful peer-initiated + // shutdown (per #1913 shape (b)), use NO_ERROR + a distinct + // "gracefully closed" message so callers can distinguish from + // forceful termination. Pending operations (e.g. pending + // `connection.ping()` calls or pending settings ACKs) still + // complete with an error — they cannot succeed across a peer + // shutdown — but the error itself carries the graceful signal. + var exception = + isGracefulShutdown + ? TransportConnectionException( + ErrorCode.NO_ERROR, + 'Connection gracefully closed by peer.', + ) + : TransportConnectionException( + errorCode, + 'Connection is being forcefully terminated.', + ); // Close all streams & stream queues _streams.terminate(exception); diff --git a/pkgs/http2/test/transport_test.dart b/pkgs/http2/test/transport_test.dart index 72aedf9ae7..e63d55603c 100644 --- a/pkgs/http2/test/transport_test.dart +++ b/pkgs/http2/test/transport_test.dart @@ -588,6 +588,43 @@ void main() { clientSettings: const ClientSettings(streamWindowSize: 8096), ); }); + + // Regression test for issue #1913: when a peer sends GOAWAY with + // NO_ERROR and then closes the underlying transport, pending + // operations on the local connection should error with a + // distinguishable graceful-shutdown signal — not with the + // "Connection is being forcefully terminated." message reserved for + // forceful termination paths. Per issue body shape (b) + + // brianquinlan's vote: the graceful-shutdown signal is + // breaking-cleaner than the prior conflated message. + transportTest('graceful-server-finish-distinct-shutdown-signal', ( + TransportConnection client, + TransportConnection server, + ) async { + // Race a pending client.ping() with server-side graceful shutdown. + // Under the prior behavior the ping errored with + // "Connection is being forcefully terminated."; per shape (b) the + // ping errors with the distinct graceful-shutdown signal. + var clientPingError = client.ping().catchError( + expectAsync2((Object e, StackTrace s) { + expect(e, isA()); + // Graceful-shutdown signal: errorCode == NO_ERROR + distinct + // message. Either condition is sufficient for callers to + // distinguish from the forceful-termination path; we assert + // both for regression safety. + final tce = e as TransportConnectionException; + expect(tce.errorCode, 0); // ErrorCode.NO_ERROR + expect( + tce.toString(), + isNot(contains('forcefully terminated')), + reason: + 'Graceful close must not surface forceful-termination text.', + ); + }), + ); + await server.finish(); + await clientPingError; + }); }); }