Reject unary calls when the http/2 stream closes with NO_ERROR before the response - #1747
Open
jackyzha0 wants to merge 1 commit into
Open
Reject unary calls when the http/2 stream closes with NO_ERROR before the response#1747jackyzha0 wants to merge 1 commit into
jackyzha0 wants to merge 1 commit into
Conversation
… the response The "close" handler in h2Request() only errors the sentinel when the stream's reset code maps to a ConnectError, and connectErrorFromH2ResetCode() intentionally maps NO_ERROR (0x0) to undefined, since that is also the code of ordinary stream completion. But when a stream is reset with NO_ERROR before response headers were received - proxies do this during graceful shutdowns - there is no ordinary completion: the response promise has not resolved, and nothing else errors the sentinel. The returned promise stays pending forever. Track whether the "response" event was seen, and reject with Code.Unavailable when the stream closes without it and without a mapped reset error. Unavailable matches the REFUSED_STREAM mapping: the server provably did not process the request, so it is safe to retry. Streams that received a response are unaffected. Fixes connectrpc#1678. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Jacky Zhao <j.zhao2k19@gmail.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
1 participant
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.
Background
Fixes #1678.
The
closehandler inh2Request()only errors the sentinel when the stream's reset code maps to a ConnectError, andconnectErrorFromH2ResetCode()intentionally mapsNO_ERROR(0x0) toundefined, because that is also the code of ordinary stream completion.But when a stream is reset with
NO_ERRORbefore response headers were received - proxies like Linkerd or Envoy do this during graceful shutdowns - there is no ordinary completion: the response promise has not resolved, and nothing else errors the sentinel. The returned promise stays pending forever, which matches the forensics in #1678 (one orphanedawait, session manager already moved on, no retained streams).Summary
Track whether the
responseevent was seen, and reject withCode.Unavailablewhen the stream closes without it and without a mapped reset error. Streams that received a response are unaffected, so ordinary completions (which also close with code 0x0) don't change behavior.Unavailable matches the existing
REFUSED_STREAMmapping: the server provably did not process the request, so it is safe to retry.The new test reproduces the hang with a server that resets the stream with
NGHTTP2_NO_ERRORwithout responding; without the fix, the response promise never settles.Related, but distinct from #1746: that one fixes a session-level hang in
Http2SessionManager.verify(); this one fixes a call-level hang in the universal client.