Repository navigation
Conversation
This branch has not been deployed
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.
Fixes #6187
Problem
The HTTP/2 abort handler in
writeH2calledstream.close()(NO_ERROR) and thenutil.destroy(stream). While the request body is still queued behind flow control, Node defers the RST_STREAM until 'finish', which never comes; the destroy then drops the stream in Node while nghttp2 still holds it open with an active data provider. Once the server grants window, the session loops on empty DATA frames until the process is OOM-killed.Fix
Give each
writeH2call its ownAbortController, pass its signal tosession.request(), and abort it in the abort handler instead of callingclose(). Node then destroys the stream with an AbortError, submitting RST_STREAM(CANCEL) first. The synchronous destroy is kept as a fallback (#5558). The user's signal is no longer passed to Node — it already drives this abort handler viarequest.onRequestStart(), and the resulting AbortError 'error' on the stream is absorbed by the existingonErrorhandler (the request is already marked aborted).Alternatives from #6187 were not used:
close(NGHTTP2_CANCEL)still runs away for streaming bodies, anddestroy(stream, err)sends INTERNAL_ERROR instead of CANCEL.Tests
New regression tests in
test/http2-abort.jsabort a request while its body (Buffer and streaming variants) is queued behind flow control against a server that never reads, and assert the server promptly receives RST_STREAM(CANCEL) and that the session stays usable. The other http2/h2 test files pass (a few files — h2-disconnect-guard, http2-connection, http2-dispatcher, http2-trailers — fail/hang identically on the unpatched baseline in my Windows environment).