Repository navigation
[v24.x backport] http2: send deferred RST_STREAM on stream destroy - #67445
Draft
gabedemesa wants to merge 2 commits into
Draft
gabedemesa wants to merge 2 commits into
gabedemesa wants to merge 2 commits into
Conversation
close() with NO_ERROR waits for 'finish' before sending RST_STREAM. If the body is stuck behind flow control and the stream is destroyed first, RST_STREAM is never sent and nghttp2 keeps the stream open. On the next WINDOW_UPDATE, OnRead finds no stream and returns 0 without EOF, so nghttp2 sends empty DATA frames until OOM. Both client and server streams are affected. Send the deferred RST_STREAM in _destroy(). As a backstop, return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE from OnRead when the stream is gone, which nghttp2 documents as the way to reset a stream on error. Fixes: nodejs#67363 Signed-off-by: inoway46 <inoueyuya416@gmail.com>
Collaborator
|
Review requested:
|
Contributor
|
Welcome to Node.js, and thank you for your first contribution! Before review, please take a moment to read:
Please make sure every commit is signed off. For a first pull request, GitHub Actions require collaborator approval and Jenkins CI must be started by a collaborator or triager, so an initial wait is normal. |
On v24.x, a clean RST_STREAM received before the body ends closes the stream without an error, unlike main, where it errors with ERR_HTTP2_STREAM_ABORTED. Expect no error on the receiving side. The tests still run out of memory without the fix. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: gdemesa <gdemesa@opengov.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
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.
Backport of #67372 to v24.x.
#67372 has not landed yet; this draft is based on its current commit (7288a16) and will be updated with the landed commit, including its PR-URL and Reviewed-By metadata, once it merges.
The cherry-pick conflicted in
lib/internal/http2/core.js, becauseonStreamClose()andcloseStream()were restructured onmainafter v24.x branched. The same changes are applied to the v24.x versions of those functions:STREAM_FLAGS_RST_DEFERREDis set wherecloseStream()defersfinishCloseStreamto'finish', and cleared infinishCloseStream().Http2Stream#_destroy()submits the deferred RST_STREAM when the stream is already closed with the flag set.onStreamClose()clears the flag before destroying. Onmainthis is done only in thelocallyInitiatedbranch; on v24.x it is done unconditionally, which is equivalent because the flag can only be set on a stream thatcloseStream()has already marked closed.The
src/node_http2.ccchange and both tests applied cleanly. Authorship is kept with the original author.The tests needed one adjustment, in a separate
fixup!commit. Onmain, a stream that receives a clean RST_STREAM before its body ends errors withERR_HTTP2_STREAM_ABORTED; on v24.x it closes without an error. The tests expected the error, so on v24.x they expect no error on the receiving side instead. Both sides still close withrstCode0, as the tests assert.Verification (macOS arm64, local build of this branch)
test-http2-close-destroy-queued-bodyandtest-http2-server-close-destroy-queued-bodypass 50 out of 50 runs each.lib/internal/http2/core.jsandsrc/node_http2.ccreverted), both tests run away, reaching 1,756 MB and 1,936 MB of RSS before being stopped.parallel/test-http2-*andsequential/test-http2-*pass.fetch()uploads aborted mid-body, a server that holds the flow-control window until the client'sAbortSignal.timeout()fires, and plainnode:http2close()followed bydestroy()(respectively 207 MB, 63 MB, under 1 MB).🤖 Generated with Claude Code