fix(call): map GOAWAY NO_ERROR to UNAVAILABLE instead of UNKNOWN - #827
fix(call): map GOAWAY NO_ERROR to UNAVAILABLE instead of UNKNOWN#827aki1770-del wants to merge 2 commits into
Conversation
When a tonic server performs a graceful shutdown it sends GOAWAY with error code NO_ERROR (0). The Dart http2 library surfaces this as TransportConnectionException(0, 'Connection is being forcefully terminated.') on each active stream. _onResponseError had no handling for this type, so it fell through to GrpcError.unknown — a non-retryable error that surfaces to callers as code 2 / UNKNOWN. Add a TransportConnectionException import and detect errorCode == 0 (NO_ERROR) in _onResponseError, mapping it to GrpcError.unavailable instead. UNAVAILABLE is the correct semantic (server temporarily unavailable, retry on new connection) and activates standard retry middleware. Non-zero error codes retain the existing UNKNOWN mapping. Fixes grpc#802. Co-Authored-By: Claude and aki1770-del <aki1770@gmail.com>
…ping (grpc#802) Inject TransportConnectionException(errorCode: 0) via mock transport stream and assert the response stream emits GrpcError.unavailable (StatusCode 14), not GrpcError.unknown (StatusCode 2). A second test verifies the non-zero errorCode path still produces a GrpcError. Co-Authored-By: Claude and aki1770-del <aki1770@gmail.com>
|
Hello — following up on this PR opened 24 days ago. CI is green and there has been no review activity yet; happy to address any feedback when a maintainer has bandwidth. The change unblocks transparent retry on graceful server shutdown: GOAWAY with NO_ERROR currently surfaces as UNKNOWN, which standard gRPC retry middleware does not retry, so callers see opaque failures during routine server restarts (e.g. tonic). Mapping to UNAVAILABLE matches the situation and is gated to errorCode == 0 only. A concrete downstream consumer is kuksa_dart_sdk (https://github.com/aki1770-del/kuksa_dart_sdk), a Dart/Flutter client for the Eclipse KUKSA Vehicle Abstraction Layer that streams from a databroker subject to graceful restarts. Three options that may help:
|
| // Map to UNAVAILABLE so retry middleware can reconnect transparently | ||
| // instead of surfacing UNKNOWN to the caller. Any non-zero errorCode is a | ||
| // real connection failure and retains the UNKNOWN mapping below. | ||
| if (error is TransportConnectionException && error.errorCode == 0) { |
There was a problem hiding this comment.
I think there are several issues with this fix:
- I think there is probably a bug in the
http2package - ifGOAWAYwithNO_ERRORis specified as graceful shutdown, then it should not translate intoTransportConnectionExceptionto begin with. I think the code inhttp2package should change to reflect this, unless there is some strong reason to not do that. Though I have trouble seeing what that reason would be... Exceptions should be reserved for error situations. - I think this is the wrong layer to check for HTTP/2 transport specific connection issues. This needs to happen in the
Http2ClientConnection. - I think connection going away should only be an error for unary calls which did not receive response, all other cases this is just a graceful shutdown of the underlying connection.
|
Agreed on all three points. On (1): if On (2): even as a defensive in-tree mapping, On (3): mapping every connection-close to Plan: closing this PR. Opening an issue on One correction: the PR description says "Closes #802." It shouldn't — #802 reports |
|
Filed: dart-lang/http#1913 |
…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).
Problem
When a gRPC server (e.g. tonic) performs a graceful shutdown it sends a
GOAWAYframe with error codeNO_ERROR(0). The Darthttp2librarysurfaces this as:
on every active stream. In
ClientCall._onResponseError, this exceptionhas no special handling and falls through to:
GrpcError.unknownis code 2 / UNKNOWN — not retryable by standardgRPC retry middleware. Callers see an opaque
UNKNOWNerror instead ofthe retryable
UNAVAILABLEthat the situation warrants.Closes #802.
Fix
Import
TransportConnectionExceptionfrom thehttp2package and addan explicit check in
_onResponseError:Non-zero
errorCodevalues (real connection failures) retain theexisting
UNKNOWNmapping.Verification
AI-assisted — authored with Claude, reviewed by Komada.