fix(h2): requeue request on GOAWAY'd session instead of crashing#5453
Open
staylor wants to merge 1 commit into
Open
fix(h2): requeue request on GOAWAY'd session instead of crashing#5453staylor wants to merge 1 commit into
staylor wants to merge 1 commit into
Conversation
When a request is dispatched onto an HTTP/2 session that has already received a GOAWAY frame, ClientHttp2Session.request() throws ERR_HTTP2_GOAWAY_SESSION. requestStream caught it and fell through to session.destroy(err) + util.destroy(socket, err) + abort(err); the destroy-with-error on a socket whose 'error' listener is already detached re-emits 'error' and crashes the process via uncaughtException (observed in production on a high-volume h2 client to servers that routinely GOAWAY to rotate connections). A GOAWAY'd session is the same situation as ERR_HTTP2_INVALID_SESSION — the peer won't accept new streams — so route it through the existing graceful branch (resetHttp2Session + requeueUnsentRequest) so the request retries on a fresh connection. Adds a regression test (stubs session.request to throw ERR_HTTP2_GOAWAY_SESSION); it fails on main and passes with the fix. Signed-off-by: Scott Taylor <scott.c.taylor@mac.com> Assisted-By: devx/86ca8ae7-2b51-4dd3-8382-dc6116301425
fd48680 to
1b329b2
Compare
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
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.
Issue
When a request is dispatched onto an HTTP/2 session that has already received a
GOAWAYframe,ClientHttp2Session.request()throwsERR_HTTP2_GOAWAY_SESSION("New streams cannot be created after receiving a GOAWAY"). InwriteH2'srequestStream, onlyERR_HTTP2_INVALID_SESSIONis handled gracefully;GOAWAYfalls through to:The
util.destroy(socket, wrappedErr)re-emits'error'on a socket whose'error'listener can already be detached during teardown, which surfaces as anuncaughtExceptionand crashes the process:Observed in production on a high-volume
allowH2: trueclient talking to servers that routinely sendGOAWAYto rotate connections: under load a queued request is dispatched onto a session in the window after theGOAWAYframe is received but beforeonHttp2SessionGoAwayretires the session, and the process crashes. Same family as the recently-hardened h2 teardown crashes (#5440, #4564).Fix
A
GOAWAY'd session is the same situation asERR_HTTP2_INVALID_SESSION— the peer won't accept new streams — so route it through the existing graceful branch (resetHttp2Session+requeueUnsentRequest). The request is reset and requeued on a fresh connection instead of failing and risking the unhandled socket'error'.Test
Added a regression test in
test/http2-goaway.js, modeled on the existinghttp2-invalid-session.jspattern: it stubshttp2.connectto return a fake session whoserequest()throwsERR_HTTP2_GOAWAY_SESSION, then asserts the request is requeued onto a fresh real session and succeeds (connectCalls === 2,200,'ok'). It fails onmain(the throw escapes via the destroy+abort fallthrough) and passes with this change.