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; + }); }); }