Repository navigation
[v26.x backport] http2: send deferred RST_STREAM on stream destroy - #67444
Draft
gabedemesa wants to merge 3 commits into
Draft
gabedemesa wants to merge 3 commits into
gabedemesa wants to merge 3 commits into
Conversation
On Linux an addon or FFI library inside a VFS is loaded from an anonymous memfd through /proc/self/fd/N, and the memfd was closed right after a successful load. The next image then got the same fd number, hence the same path, and the dynamic loader, which knows a loaded object by the path it was opened with, returned the object already loaded under that path instead of loading the new file. So the second of any two addons or libraries loaded from a VFS resolved to the first: an addon after an FFI library failed with "Module did not self-register", an FFI library after an addon had none of its own symbols, and two addons silently became the same one. The fd is only needed until dlopen() returns, as the load's mapping keeps the memory alive from then on; what must not be shared is the path. Before the load, ask the loader with RTLD_NOLOAD whether the path is already taken, and if so move the fd to a higher number with F_DUPFD_CLOEXEC until it is free. Holding the fd from the check through the load keeps anyone else from taking the path in between, so this needs neither a counter nor a lock, and a library that has really been unloaded gives its number back. The fd is closed right after the load, whether or not it worked. Should no free number be left, the image goes to a temp file. The leak checks of test-vfs-addon and test-ffi-vfs compared the shared temporary directory before and after the load, so off Linux any other test loading from bytes at the same time could pass for a leak. Point TMPDIR at a directory each test owns instead. Signed-off-by: Philipp Dunkel <pip@pipobscure.com> PR-URL: nodejs#67068 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Filip Skokan <panva.ip@gmail.com>
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:
|
nodejs-github-bot
added
c++
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. |
aduh95
force-pushed
the
v26.x-staging
branch
from
November 13, 2026 08:03
c2060d3 to
e7c0957
Compare
gabedemesa
force-pushed
the
backport-67372-to-v26.x
branch
from
November 13, 2026 16:33
cb93cb2 to
007f81c
Compare
On v26.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. Assisted-by: a closed-source coding agent Signed-off-by: Gabe De Mesa <gdemesa@opengov.com>
gabedemesa
force-pushed
the
backport-67372-to-v26.x
branch
from
November 13, 2026 16:41
007f81c to
ad86579
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.
Backport of #67372 to v26.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 v26.x branched. The same changes are applied to the v26.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 v26.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 v26.x it closes without an error. The tests expected the error, so on v26.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,627 MB and 1,866 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 136 MB, 70 MB, under 1 MB).