You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
node:http2: throw from respond(), respondWithFD() and respondWithFile() on a closed stream - #48439
Since bun test: Vitest compatibility #44833, stream.close(code) followed by stream.respond() sends the response. The client gets 'response' with status 200 and rstCode 0, for every code. Bun 1.4.4 sent no response. Node throws ERR_HTTP2_INVALID_STREAM.
respond(), respondWithFD() and respondWithFile() test destroyed only (src/js/node/http2.ts:3600, :3483, :3423). A stream that close(code) closed is not destroyed yet.
Fix
The three methods throw ERR_HTTP2_INVALID_STREAM for a stream that is destroyed or closed. Node has the same test in all three.
doSendFileFD cancels a file response when its statCheck closed or destroyed the stream. Without that, the new test throws inside the fstat callback.
Verified: test/js/node/http2/node-http2-client-close.test.ts (15 new cases, main fails all 15). Also test/js/node/http2/, 279 vendored test-http2-*, grpc-js.
Self-reviewed as part of a larger change: 2 concerns raised about these lines, 2 addressed (Notes).
Background
close(code) marks the stream closed and sends RST_STREAM. The stream is destroyed later: after 'finish' for code 0, after the reset for other codes.
A file response answers in steps: fstat(), the user's statCheck, then respond().
respond*() on a closed stream throws. On 1.4.4 it returned and sent nothing, so close() then respond() with no try now raises, as in node.
Http2Server's captureRejections handler calls respond(). For a rejected 'stream' listener that closed its stream first, it now throws, as in node.
Each respond*() call reads closed once more: +2 bytecodes per call, +60 bytes of module text.
Notes
close(code), then respond() and end("ok") in the same turn. Node is v26.12.1. Main is 2596ac5, the base this was measured on. Release builds.
node close(0): respond() throws ERR_HTTP2_INVALID_STREAM ; client end (0 bytes) ; client close:0
close(8): respond() throws ERR_HTTP2_INVALID_STREAM ; client end (0 bytes) ; client close:8
close(2): respond() throws ERR_HTTP2_INVALID_STREAM ; client error:ERR_HTTP2_STREAM_ERROR ; client close:2
close(7): respond() throws ERR_HTTP2_INVALID_STREAM ; client error:ERR_HTTP2_STREAM_ERROR ; client close:7
1.4.4 close(0), close(8), close(2), close(7): respond() returns ; client end (0 bytes) ; client close:0
main close(0), close(8), close(2), close(7): respond() returns ; client response 200 ; client end (0 bytes) ; client close:0
this PR the four lines of node
respondWithFile() whose statCheck closes or destroys the stream. Each line is the same with and without an onError option, except where it names onError.
node close(8): server [close:8] client [close:8]
close(2): server [error:ERR_HTTP2_STREAM_ERROR, close:2] client [error:ERR_HTTP2_STREAM_ERROR, close:2]
destroy(): server [close:0] client [close:0]
main close(8): server [close:8] client [response 200, close:0]
close(2): server [error:ERR_HTTP2_STREAM_ERROR, close:2] client [response 200, close:0]
destroy(): server [close:0] client [close:0], and onError(ERR_HTTP2_INVALID_STREAM) when there is one
this PR the three lines of node
With the three tests and no change in doSendFileFD, the close(8) row is server [error:ERR_HTTP2_INVALID_STREAM, close:8], or onError(ERR_HTTP2_INVALID_STREAM). doSendFileFD calls the public respond() after the statCheck. Node's doSendFileFD calls processRespondWithFD(), which has no such test.
Node.respond(), respondWithFD() and respondWithFile() start with if (this.destroyed || this.closed) throw new ERR_HTTP2_INVALID_STREAM(). additionalHeaders() in http2.ts already has this test.
The second text.src/js/ahead-of-time/node/http2.ts is the text of this module that bun build --compile --aot reads. It has the same four lines, and the new hash of the first text at its head (test/internal/second-texts-of-modules.test.ts).
Self-review. The review was of a larger change for RST_STREAM(NO_ERROR) that had these lines. It raised two concerns about them. First: the three tests stand alone and can land by themselves. This PR is that cut. Second: doSendFileFD was a caller in the tree that the new test broke. The statCheck line and its 6 cases are the answer.
Tests. 15 new cases in the describe respond() after close(code). All 15 fail on main, and no older case does. Each of the four changed lines was removed once, and a case failed each time. The file also runs under node --test: node 26.12.1 passes 88 and skips 6. The 6 are the statCheck cases. Node starts to read the file when the statCheck returns, and node 26 can crash when the stream closes before that read is done (SIGSEGV in the callback of FileHandle::ReadStart()). The table above has node's lines for them.
Measurements. Release builds of main at 2596ac5, without and with this diff.
Reads of closed per server stream: +1 for a GET, a POST, the compat API and an implicit response. +3 for a file response. +0 on client streams. (2,000 streams each, a counting accessor.)
#46528. That PR has the same three lines at http2.ts:3561, :3625 and :3740. A three-way merge of this PR into its head (edb9ce8) has 49 conflict regions in http2.ts, the same 49 as main alone (98cda0d). Its doSendFileFD does not have the statCheck line: a build of edb9ce8 prints server [error:ERR_HTTP2_INVALID_STREAM, close:8] for the close(8) row. The two file methods' lines are also in #43564, #43463 and #43550, for other bugs.
Suites on the debug build.node-http2-client-close.test.ts 95 pass. h2-conformance.test.ts 143 pass. node-http2.test.js 419 pass, 7 skip. The 8 other files of test/js/node/http2/ pass. Vendored test-http2-*.js and test-diagnostics-channel-http2-*.js: 279 of 279. serve-http2*.test.ts 317 pass, fetch-http2-client.test.ts 88, undici-h2 11, wpt-h2 20, http2-wrapper 1. grpc-js: 26 of 29 files pass. test-resolver (public DNS) and test-tonic fail the same on main. test-outlier-detection is timed and fails 1 case on a debug build with or without this change.
no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/http2/node-http2-client-close.test.ts
Draft. It waits for a decision on the order with #46528 (note).
How I reproduced it. A server stream calls stream.close(code) and then stream.respond({ ":status": 200 }) in the same turn. A client request records 'response', 'error' and 'close'. The codes are 0, 8, 2 and 7.
main (2596ac5d39) respond() returns ; client response 200 ; client close:0
Bun 1.4.4 respond() returns ; client close:0
Node v26.12.1 respond() throws ERR_HTTP2_INVALID_STREAM ; client close:<code>
this PR the same as Node
The cases are in test/js/node/http2/node-http2-client-close.test.ts, in the describe respond() after close(code). 15 fail on main, and 15 pass here.
CI. Build 139642 has no failure from this diff. 184 jobs passed, and 4 macOS x64 test jobs wait for an agent. Three unrelated tests (gc-controller-cadence.test.ts, bun-serve-html.test.ts, hot.test.ts) failed once and passed alone or on retry.
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
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.
Problem
stream.close(code)followed bystream.respond()sends the response. The client gets'response'with status 200 andrstCode0, for every code. Bun 1.4.4 sent no response. Node throwsERR_HTTP2_INVALID_STREAM.respond(),respondWithFD()andrespondWithFile()testdestroyedonly (src/js/node/http2.ts:3600,:3483,:3423). A stream thatclose(code)closed is not destroyed yet.Fix
ERR_HTTP2_INVALID_STREAMfor a stream that is destroyed or closed. Node has the same test in all three.doSendFileFDcancels a file response when itsstatCheckclosed or destroyed the stream. Without that, the new test throws inside thefstatcallback.test/js/node/http2/node-http2-client-close.test.ts(15 new cases, main fails all 15). Alsotest/js/node/http2/, 279 vendoredtest-http2-*, grpc-js.Background
close(code)marks the stream closed and sendsRST_STREAM. The stream is destroyed later: after'finish'for code 0, after the reset for other codes.fstat(), the user'sstatCheck, thenrespond().Downsides
respond*()on a closed stream throws. On 1.4.4 it returned and sent nothing, soclose()thenrespond()with notrynow raises, as in node.Http2Server'scaptureRejectionshandler callsrespond(). For a rejected'stream'listener that closed its stream first, it now throws, as in node.respond*()call readsclosedonce more: +2 bytecodes per call, +60 bytes of module text.Notes
close(code), thenrespond()andend("ok")in the same turn. Node is v26.12.1. Main is 2596ac5, the base this was measured on. Release builds.respondWithFile()whosestatCheckcloses or destroys the stream. Each line is the same with and without anonErroroption, except where it namesonError.With the three tests and no change in
doSendFileFD, theclose(8)row isserver [error:ERR_HTTP2_INVALID_STREAM, close:8], oronError(ERR_HTTP2_INVALID_STREAM).doSendFileFDcalls the publicrespond()after thestatCheck. Node'sdoSendFileFDcallsprocessRespondWithFD(), which has no such test.Node.
respond(),respondWithFD()andrespondWithFile()start withif (this.destroyed || this.closed) throw new ERR_HTTP2_INVALID_STREAM().additionalHeaders()inhttp2.tsalready has this test.The second text.
src/js/ahead-of-time/node/http2.tsis the text of this module thatbun build --compile --aotreads. It has the same four lines, and the new hash of the first text at its head (test/internal/second-texts-of-modules.test.ts).Self-review. The review was of a larger change for
RST_STREAM(NO_ERROR)that had these lines. It raised two concerns about them. First: the three tests stand alone and can land by themselves. This PR is that cut. Second:doSendFileFDwas a caller in the tree that the new test broke. ThestatCheckline and its 6 cases are the answer.Tests. 15 new cases in the describe
respond() after close(code). All 15 fail on main, and no older case does. Each of the four changed lines was removed once, and a case failed each time. The file also runs undernode --test: node 26.12.1 passes 88 and skips 6. The 6 are thestatCheckcases. Node starts to read the file when thestatCheckreturns, and node 26 can crash when the stream closes before that read is done (SIGSEGV in the callback ofFileHandle::ReadStart()). The table above has node's lines for them.Measurements. Release builds of main at 2596ac5, without and with this diff.
respond(): 373 -> 375 bytecodes.respondWithFD(): 174 -> 176.respondWithFile(): 130 -> 132.doSendFileFD: 375 -> 377.closedper server stream: +1 for a GET, a POST, the compat API and an implicit response. +3 for a file response. +0 on client streams. (2,000 streams each, a counting accessor.)node:http2builtin: 172,226 -> 172,286 bytes. Release binary text: +60 bytes. Data: +0.#46528. That PR has the same three lines at
http2.ts:3561,:3625and:3740. A three-way merge of this PR into its head (edb9ce8) has 49 conflict regions inhttp2.ts, the same 49 as main alone (98cda0d). ItsdoSendFileFDdoes not have thestatCheckline: a build of edb9ce8 printsserver [error:ERR_HTTP2_INVALID_STREAM, close:8]for theclose(8)row. The two file methods' lines are also in #43564, #43463 and #43550, for other bugs.Suites on the debug build.
node-http2-client-close.test.ts95 pass.h2-conformance.test.ts143 pass.node-http2.test.js419 pass, 7 skip. The 8 other files oftest/js/node/http2/pass. Vendoredtest-http2-*.jsandtest-diagnostics-channel-http2-*.js: 279 of 279.serve-http2*.test.ts317 pass,fetch-http2-client.test.ts88,undici-h211,wpt-h220,http2-wrapper1.grpc-js: 26 of 29 files pass.test-resolver(public DNS) andtest-tonicfail the same on main.test-outlier-detectionis timed and fails 1 case on a debug build with or without this change.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/http2/node-http2-client-close.test.ts