Repository navigation
Conversation
close(0), close(8) and close() of a pending stream give the readable its EOF first. They set the Closed bit last, after the 'aborted' listeners and end(). pushToStream() drops DATA only for a closed stream, so a DATA frame that the session parsed in between reached stream.push() after EOF: 'error' ERR_STREAM_PUSH_AFTER_EOF, and for close(0) a stream that never closed. A session parses a frame inside close() when its transport runs in the same process: inside a listener that close() runs, or inside the write of the END_STREAM when the peer answers at once (a duplexPair() side). close() now sets a status bit, EndedByClose, before it ends the readable. pushToStream() tests it in the mask that it already tests for Closed.
|
Updated 12:05 AM PT - Oct 22nd, 2026
❌ @robobun, your commit fdfe04b has some failures in 🧪 To try this PR locally: bunx bun-pr 46428That installs a local version of the PR into your bun-46428 --bun |
|
Status Reproduced with two sessions on a repro.cjsconst http2 = require("node:http2");
const { duplexPair } = require("node:stream");
const { once } = require("node:events");
(async () => {
const [clientSide, serverSide] = duplexPair();
const server = http2.createServer();
server.on("stream", stream => {
stream.on("error", () => {});
// The request has no DATA, so the first 'readable' is its EOF: answer inside that event.
stream.on("readable", () => {
while (stream.read() !== null);
if (stream.headersSent) return;
stream.respond({ ":status": 200 });
stream.end("ok");
});
});
server.emit("connection", serverSide);
const client = http2.connect("http://localhost", { createConnection: () => clientSide });
client.on("error", () => {});
for (const code of [0, 8]) {
const opened = once(server, "stream");
const req = client.request({ ":method": "POST", ":path": "/" });
await opened;
const events = [];
const closed = new Promise(resolve => req.on("close", resolve));
req.on("error", err => events.push("error:" + err.code));
req.on("response", headers => events.push("response:" + headers[":status"]));
req.on("data", chunk => events.push("data:" + chunk));
for (const name of ["aborted", "end", "finish"]) req.on(name, () => events.push(name));
req.on("close", () => events.push("close:" + req.rstCode));
req.close(code);
await closed;
console.log(`close(${code}): ${events.join(" | ")}`);
}
client.destroy();
server.close();
})();The fix is in #46428. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
Walkthrough
ChangesHTTP/2 stream close handling
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly and concisely describes the primary change: dropping DATA that arrives after close(code) ends the readable side. |
| Description check | |
The description explains the problem, fix, behavioral scope, limitations, related work, and verification results. It does not use the exact template headings, but it provides the required information … |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
Full details: Description check
-
Explanation
The description explains the problem, fix, behavioral scope, limitations, related work, and verification results. It does not use the exact template headings, but it provides the required information and is complete.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked the other push(null) sites in src/js/node/http2.ts for the same EOF-then-DATA window: the session-teardown push (line 2295) is gated on NativeClosed, so no further DATA frame can arrive for that stream, and the _destroy/_read pushes only run on an already-destroyed stream, where Readable's errorOrDestroy is a no-op — so close() is the only site that needed the new bit.
Extended reasoning...
The diff adds one StreamState.EndedByClose bit set in Http2Stream.close() before push(null) and widens the mask pushToStream tests, plus ~190 lines of node:test compat tests driving client and server sessions over a test-owned Duplex and a duplexPair. It touches no auth, crypto, or injection surface. Two confirmed inline findings (a matrix test that can hang rather than fail, and _final overwriting the status word after its diagnostics channel can run close()) mean a human should weigh the change; this note only records the sibling-site check that was ruled out.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/js/node/http2.ts— Users whose diagnostics subscriber closes the stream still get ERR_STREAM_PUSH_AFTER_EOF, the error this PR says it removes. _final reads the status word at src/js/node/http2.ts:2714, publishes the bodySent channel at 2717, then overwrites the word with the stale copy at 2720. A close() run by the subscriber sets EndedByClose and Closed in between, and both bits are erased. The next DATA frame passes the mask at 2029 and reaches push() after EOF. Fix: make 2720 an|=on the live word (and treat every read-publish-write of the status word the same way), so no bit set by re-entrant user code is lost.Why this was flagged
Trigger: a stream whose final DATA already carried END_STREAM (EndStreamSent set, src/js/node/http2.ts:2719) reaches _final, and a subscriber of onClientStreamBodySentChannel (2716-2717) calls stream.close(0) or close(8). close() sets EndedByClose at 2580, pushes EOF at 2581, then markStreamClosed sets Closed at 2225. Back in _final, line 2720 writes
status | FinalCalledfrom the copy read at 2714, which has neither bit. pushToStream at 2029 now sees a word with no Closed and no EndedByClose; the peer's next DATA frame is pushed into an ended readable and readable.ts:430 emits ERR_STREAM_PUSH_AFTER_EOF, exactly the error the PR exists to stop, and a second close() is also accepted because 2562 sees Closed clear. The dismissal called this pre-existing because Closed was already lost there; but the PR adds a second guard bit to the same lossy write-back and claims to fix the whole class, and the author lists this hole in the PR notes rather than fixing it. The remedy is a one-line change to OR into the live word at 2720.Verification: Pre-existing; the base fails by the same route. Trigger: _final publishes to onClientStreamBodySentChannel, a subscriber calls close(0), and the peer sends a DATA frame before the stream is destroyed. close() sets EndedByClose and Closed, but src/js/node/http2.ts:2720 writes the stale snapshot, erasing both; the guard at 2029 tests 0 and push() at 2044 raises ERR_STREAM_PUSH_AFTER_EOF.
| const { promise: settled, resolve } = Promise.withResolvers<void>(); | ||
| let closing = false; | ||
| const feedOnce = () => { | ||
| if (!closing) return; | ||
| closing = false; | ||
| receive(FRAME.DATA, 0, Buffer.from("late")); | ||
| }; | ||
| if (listener === "readable") { | ||
| stream.on("readable", () => { | ||
| while (stream.read() !== null); | ||
| feedOnce(); | ||
| }); | ||
| stream.read(); | ||
| } else { | ||
| stream.on(listener, feedOnce); | ||
| stream.resume(); | ||
| } | ||
| stream.on("error", err => { | ||
| const { code } = err as NodeJS.ErrnoException; | ||
| events.push("error:" + code); | ||
| // This error can leave the stream open, with no 'close'. | ||
| if (code === "ERR_STREAM_PUSH_AFTER_EOF") resolve(); | ||
| }); | ||
| stream.on("close", () => { | ||
| events.push("close:" + stream.rstCode); | ||
| resolve(); | ||
| }); | ||
| closing = true; | ||
| stream.close(code); | ||
| await settled; |
There was a problem hiding this comment.
🔴 A regression in close() that stops 'close' from firing makes these 24 matrix tests hang to the runner timeout instead of failing with a message. The settled promise at test/js/node/http2/node-http2-client-close.test.ts:440 is only resolved, by 'close' at 465 or by ERR_STREAM_PUSH_AFTER_EOF at 461. No session 'error', stream 'error' with another code, or transport end rejects it, and the await at 469 has no bound. Fix: wire every failure event (session 'error', unexpected stream 'error' codes, transport 'close') to reject settled, as REVIEW.md requires for every awaited condition.
Why this was flagged
Trigger: any future change that leaves close(0)/close(8) without a 'close' event and without ERR_STREAM_PUSH_AFTER_EOF, for example a regression that drops the scheduled destroy at src/js/node/http2.ts:2606-2610. The test attaches 'error' (457) and 'close' (463) listeners but only resolves at test/js/node/http2/node-http2-client-close.test.ts:461 and 465; the Promise.withResolvers at 440 discards reject. The session created in openOnOwnedTransport (400, 403) has no 'error' listener wired to the promise, and the owned transport never closes. The await at 469 then waits for the per-test timeout, and on the debug/ASAN lane the 24 matrix cases each consume the full timeout rather than reporting the event list. REVIEW.md's 'Await conditions, wire failures to reject' rule is an explicit check, so this is not subjective style. Remedy: reject settled from session 'error', from any stream 'error' code not in expected, and from a bounded timer, so a failure produces the collected events in the assertion message.
Verification: Trigger: a regression in Http2Stream.close() that leaves close(0)/close(8) without a 'close' event and without an ERR_STREAM_PUSH_AFTER_EOF 'error'. test/js/node/http2/node-http2-client-close.test.ts line 440 discards reject; the only resolvers are lines 461 and 465; line 469 await settled; has no bound. Consequence: 24 timeouts with no assertion message instead of a prompt failure.
Problem
Http2Stream.close(0)andclose(8)emit'error'ERR_STREAM_PUSH_AFTER_EOF("stream.push() after EOF") when a DATA frame arrives whileclose()runs. Afterclose(0)the stream never emits'close'.close()ends the readable first (src/js/node/http2.ts:2577) and sets theClosedbit last (:2590).pushToStream()(:2029) drops DATA only for a closed stream.Fix
close()sets a status bit,EndedByClose, before it ends the readable.pushToStream()tests it in the mask that it already tests forClosed.close(): there only the listener case in Downsides changes.test/js/node/http2/node-http2-client-close.test.ts. 16 of 28 new tests fail on main, all pass on node v26.3.0. Alsotest/js/node/http2/and 280 vendored files.Background
duplexPair()side that call can be insideclose().pushToStream()runs for each DATA frame and tests the status word of the stream._readableState.ended: it adds a load per DATA frame.Downsides
close(0)andclose(8)drop a DATA frame that arrives inside'aborted'. Node v26.3.0 delivers it, node main drops it (http2: submit RST_STREAM before emitting 'aborted' nodejs/node#66314).'aborted'listener throws out ofclose(0), later DATA is dropped. Main emits the same'error'for it.node/http2.jsgrows by 57 bytes.close()has 9 more bytecode instructions,pushToStream()keeps its 41.Notes
Origin. Found in the review of #44848, which left it for a separate change in
pushToStream(). No user reported it. The error is in bun 1.4.0 to 1.4.3 and not in 1.3.14. With no'error'listener on the stream the process exits with code 1.Rows. Two sessions on a
duplexPair(). The server answers with a body when it reads END_STREAM:A Duplex that the test owns. The DATA frame is pushed inside the
'aborted'listener (client and server rows are equal):What the tests hold. For each role and for the listeners
'aborted','prefinish'and'readable':close(0)andclose(8)emit no'error'and reach'close'with the code. The rows for codes 2 and 11 pass on main and stay as they are. The tests do not record'data'or'end', because the node lines differ there. They pass on node v26.3.0, v26.12.0 and the v27 nightly of 2026-10-19.Still different from node.
'response'arrives before'finish', andclose(8)still delivers it.close(2)andclose(7)still deliver the response and its body insideclose(), as on main. These follow from the order ofclose(), which storesrstCodeand the closed state after its listeners. node:http2: store the code of close(code) before its listeners run #44848 changes that order._destroy()also ends the readable before it records the close. A DATA frame after that is ignored by the stream layer, because the stream is destroyed. No error on main, none here.perf_hooksbytesReadof the stream does not count a dropped DATA frame. This is the same as for a frame that arrives afterclose()returned._finalwrites back the status word that it read before it published to its diagnostics channel. A subscriber that callsclose()there loses the new bit, as it losesClosedon main.A listener that throws.
close(0)with an'aborted'listener that throws leaves the stream with an ended readable and no closed state, on main and here. On main the next DATA frame of the peer then emitsERR_STREAM_PUSH_AFTER_EOF, on a socket too. Here it is dropped. The stream stays open in both. #44848 makes such a close complete.The bit.
EndedByClose = 1 << 30sits next toClosedin the enum because the two are tested together. The value is not the next free one: several open PRs add a flag with the value1 << 9at the end of the enum, and two flags with one value pass the type check.Cost, measured on a debug build of main 9e4545f and of this branch (linux-x64).
pushToStream(): 41 bytecode instructions, 319 bytes on both. The listings differ in one operand, the constant of thebitand(8, now 1073741832). On arm64 the JIT can need one more instruction to load that mask (from the source of the macro assembler, not measured).Http2Stream.close(): 148 to 157 instructions. The 9 run only whenclose()ends the readable.node:http2functions keep their source hash. The other 4 arepushToStream,closeand the two module wrappers.node/http2.js: 169,604 to 169,661 bytes.The larger change that was not opened. A port of node's
closeStream()forclose()and_destroy()was built first. Its review said not to open it: it rewrites the lines that #44848 and #45056 change, and it moved three rows away from main (the AsyncLocalStorage store of'error'and'close'afterclose(8), an echo load on a synchronous pair,close(0)behind a full window). #44848 asks a maintainer to decide the order of the close path. This PR does not take that decision. Ifclose()later stores its closed state before the EOF,EndedByCloseis no longer needed and its tests still apply.Suites run (debug ASAN build, linux-x64): the changed test file (101 pass, 15 runs under node v26.3.0), the 11 files of
test/js/node/http2/(same results as main: the subprocess tests ofnode-http2.test.jsthat time out at 5 s do so with main'shttp2.tstoo), the 280 filestest/js/node/test/{parallel,sequential}/test-http2-*.js,test-diagnostics-channel-http2-*.jsandtest-stream-pipeline-http2.js,tsc --noEmit -p src/js/tsconfig.jsonand the type check oftest/.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