Repository navigation
Conversation
A server stream that user code paused was never destroyed when its readable side held no data: no 'end', no 'close'. Since #32488 the streamEnd handler of the server session waits for 'end' before destroy(), but it only resumes a stream nobody touched. Nothing ended the readable side of a paused stream. With the compat API, req.pause() on a request with no body meant res never emitted 'finish' or 'close'. When both halves have closed, the handler now calls read(0), like node's onStreamClose. read(0) ends a readable that is ended and empty. It does not consume data, so a paused stream with unread data still waits for its reader. The close can arrive inside end(), while the last write is in flight. An 'end' at that point destroys the stream before 'finish'. So the handler calls read(0) once the writable side has finished.
|
Updated 8:25 AM PT - Sep 20th, 2026
✅ @robobun, your commit a8a4926313a935b78057c8e725e07d215c6c2bda passed in 🧪 To try this PR locally: bunx bun-pr 43631That installs a local version of the PR into your bun-43631 --bun |
|
Status: ready for review. Buildkite passes on a8a4926 and no review thread is open. How I reproduced it. Run the repro script from the Notes of the PR description. A server
I ran the official release binaries for the version rows. The regression came with #32488, which first shipped in 1.4.0. Proof in the test file. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. WalkthroughThe HTTP/2 implementation adds zero-byte reads for paused server streams during closure. Tests cover lifecycle events, response completion, reset handling, compatibility-layer closure, and unread buffered request data. ChangesPaused HTTP/2 server streams
Suggested reviewers: Priority: ⬇️ Low 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/js/node/http2.ts— Client callers whose stream closes inside req.end(chunk) still lose 'finish' and get ERR_STREAM_DESTROYED in the end() callback; only the server side of this pair is fixed. The PR's own rationale for deferring read(0) until 'finish' (src/js/node/http2.ts:4064-4067) applies verbatim to the client handler at :5062, which still calls read(0) unconditionally before the once('end', destroySelfOnEnd) at :5075. Fix: apply the same writableFinished check and readNothing deferral in ClientHttp2Session.streamEnd, or state in the PR why the client cannot reach state 7 inside end(chunk); the class is one concern.Extended reasoning...
Confirmed with git show on the base: the client handler is byte-identical at :5053 (base) and :5062 (head), so this is a sibling site the diff leaves untouched, not a new line. Trigger: server answers a request with HEADERS carrying END_STREAM (e.g. an early 4xx) while the client still owes body. Client 'response' handler calls req.end('last'). The native parser has the remote half closed; the write with END_STREAM moves the stream to state 7 inside that end() call, exactly as the PR describes for the server. If streamEnd(6) was dispatched just before 'response', read(0) at :5062 has already queued endReadableNT on nextTick but endEmitted is still false, so at state 7 stream.readable is true and :5075 registers once('end', destroySelfOnEnd). The write callback for the END_STREAM chunk is deferred by kDeferWriteCallback (:2864) and lands after that nextTick. 'end' fires first, destroy() runs, Writable's kOnFinished callbacks are flushed with ERR_STREAM_DESTROYED and 'finish' never emits. The test the PR added for this on the server ("still emits 'finish' when it responds after the…
Verification: pre-existing (the client handler is untouched by this PR and the base behaves identically), but it is the sibling site of the exact hazard the PR's own new comment names, so the class is only half-fixed. Trigger: a client stream whose response arrives with END_STREAM while the request body is still open, and the user answers with req.end(chunk[, cb]) over a JS-side socket transport (duplexPair /…
|
Thank you for the review. I measured both findings on Node v26.3.0, on main (canary 1. I did not add a 2. The client handler. 12 rows: TCP and a JS socket transport ( The first matrix shows differences from Node in the close(code) on a paused server stream: output and scriptmain and this branch (identical): Node v26.3.0: // Paused server stream, request with no body (GET). The handler rejects it with close(code).
// Usage: <runtime> close-code.js
const http2 = require("node:http2");
async function run(code, respondFirst, when) {
const events = [];
let sstream;
const server = http2.createServer();
server.on("stream", s => {
sstream = s;
s.pause();
for (const ev of ["aborted", "end", "finish", "close"]) s.on(ev, () => events.push(ev));
s.on("error", e => events.push(`error:${e.code}`));
const go = () => {
if (respondFirst) s.respond({ ":status": 200 });
s.close(code);
};
if (when === "sync") go();
else setTimeout(go, 20);
});
await new Promise(r => server.listen(0, r));
const client = http2.connect(`http://127.0.0.1:${server.address().port}`);
client.on("error", () => {});
const req = client.request({ ":path": "/" });
req.on("error", () => {});
req.resume();
await new Promise(r => req.on("close", r));
await new Promise(r => client.ping(r));
await new Promise(r => setTimeout(r, 150));
const label = `close(${code}) ${respondFirst ? "after respond()" : "no respond() "} ${when.padEnd(5)}`;
console.log(` ${label} [${events.join(", ")}] destroyed=${sstream.destroyed}`);
client.destroy();
server.close();
}
(async () => {
for (const code of [0, 8, 2, 11]) {
for (const respondFirst of [false, true]) {
for (const when of ["sync", "async"]) await run(code, respondFirst, when);
}
}
process.exit(0);
})();client stream, early response: output and scriptmain and this branch (identical): // CLIENT stream. The server answers with END_STREAM on its HEADERS before the client ends its
// request. The client then calls req.end("x", cb). Does the client keep 'finish'?
// Usage: <runtime> client-early.js
const http2 = require("node:http2");
const { duplexPair } = require("node:stream");
function run(transport, when, variant) {
return new Promise(resolve => {
const events = [];
const server = http2.createServer();
server.on("stream", s => {
s.on("error", () => {});
s.respond({ ":status": 400 }, { endStream: true });
});
const start = connect => {
const client = connect();
client.on("error", () => {});
const req = client.request({ ":path": "/", ":method": "POST" });
if (variant === "paused") req.pause();
if (variant === "flowing") req.on("data", () => {});
for (const n of ["end", "finish", "close"]) req.on(n, () => events.push(n));
req.on("error", e => events.push("error:" + e.code));
const finishIt = () => req.end("x", err => events.push(err ? "endcb:" + err.code : "endcb ok"));
req.on("response", () => (when === "response" ? finishIt() : setTimeout(finishIt, 30)));
setTimeout(() => {
console.log(` ${transport.padEnd(6)} ${when.padEnd(8)} ${variant.padEnd(9)} [${events.join(", ")}] destroyed=${req.destroyed}`);
client.destroy();
server.close();
resolve();
}, 600);
};
if (transport === "tcp") {
server.listen(0, () => start(() => http2.connect(`http://127.0.0.1:${server.address().port}`)));
} else {
const [clientSide, serverSide] = duplexPair();
server.emit("connection", serverSide);
start(() => http2.connect("http://localhost:80", { createConnection: () => clientSide }));
}
});
}
(async () => {
for (const transport of ["tcp", "duplex"])
for (const when of ["response", "later"])
for (const variant of ["paused", "untouched", "flowing"]) await run(transport, when, variant);
process.exit(0);
})(); |
Problem
node:http2server stream that user code paused is never destroyed when it holds no unread data: no'end', no'close'. With the compat API,req.pause()on a GET meansresnever emits'finish'or'close'. Node v26.3.0 emits them all.streamEndhandler (src/js/node/http2.ts:4057) now waits for'end'beforedestroy(), but it resumes only a stream nobody touched. Nothing reads a paused stream, so'end'never comes. 1.3.14 destroyed every stream at the close.Fix
stream.read(0)if it does not resume, like node'sonStreamClose. The client handler already does.read(0)only ends a readable that is ended and empty. Unread data still waits for its reader.end(), with the last write in flight. An'end'then destroys the stream before'finish', soread(0)waits until the writable side has finished.test/js/node/http2/node-http2.test.js(7 new tests, 6 fail without the fix, all pass on Node),test/js/node/http2/, the 261 vendoredtest-http2-*tests. Self-review and limits: Notes.Background
Http2Streamis a Duplex withautoDestroy: false. Onlydestroy()emits'close'.streamEnd. State 7 means both halves have closed. A peer reset takes another path.pause()setsreadableFlowingtofalse. A paused Readable emits'end'only when something callsread().Notes
Repro.
Versions. Official release binaries, the version string is the one each binary reports. "main" is canary
1.4.3-canary.1+367d939d9. No 1.4.3 release exists. The last column is the compat API: 200 requests on one connection, each handler callsreq.pause()andres.end("ok").Provenance. The parent of 4bbe075 (#32488) still calls
stream.destroy()at once in the state 7 block of the server handler. 4bbe075 adds the wait for'end', which is the first half of node'sonStreamClose. This PR adds the second half. A revert is not the fix: with an unread body, 1.3.14 destroys the stream before the reader gets to it (finish, close, never'end'), where node, main and this PR givefinish, (the reader reads), end, close. The test "keeps unread data until the user reads it" guards that.Reach. I found no issue that describes this. The shapes I measured as affected all pause and then respond without a read or a
resume(), on a request with no unread data: the raw API, the compat API (req.pause()), and a 401 sent while the stream is paused for an async auth check (main:finish, stream not destroyed; this PR:finish, end, close). grpc-js 1.12.0 resumes the server streams it pauses (server-interceptors.js). The library code of fastify 5.2.2 never callspause().Server stream events, paused in the
'stream'listener. Measured. "later" means the response starts on a later turn, after the request ended.Bun emits
'finish'before'end'where node emits'end'first. An untouched stream has the same order on main (finish, end, closeagainst node'send, finish, close), so this PR keeps it. Where node prints no'finish', node destroys the stream before its writable side finishes. Bun keeps'finish'there on purpose (the existing mid-finish branch of the handler). Over a JS socket transport (duplexPair) I measured the two paused GET rows: main givesfinishand leaves the stream undestroyed, this PR givesfinish, end, close.Scope. Before the rebase I compared main with this build over 43 scenarios (untouched, flowing,
'readable'listener, unread body, pipe under backpressure, client streams, the compat API). Every scenario that differs is a paused stream with no unread data.destroySelfOnEndand the client handler are unchanged.Why
read(0)waits for'finish'. With the request already complete,end(chunk)sends END_STREAM and the native parser reports state 7 inside that call. An immediateread(0)queues'end'ahead of the deferred write callback. The existing'end'listener then callsdestroy(),'finish'never fires, and theend(chunk, cb)callback getsERR_STREAM_DESTROYED. The test "still emits 'finish' when it responds after the request ended" fails with an immediateread(0).A design I built first and dropped. It called
read(0)at once and madedestroySelfOnEndwait for'finish', like node'skMaybeDestroy. All streams share that function. Measured with awrite()afterend()from an'end'listener that user code adds after the close:The dropped design gave a stream that user code never paused a new
'error'event, which is an uncaught exception without a listener. It also left the paused stream undestroyed where node destroys it.Not covered. All measured, all the same on main and with this PR.
'end'listener hangs when the request ends on an empty DATA frame (POST,end()with no data). Node answers 200, because it also callsread(0)when END_STREAM arrives on a DATA frame (onStreamRead). 1.3.14 hangs too. With END_STREAM on the HEADERS frame (GET) node hangs as well. I left the half close alone: an'end'there would be missed by a listener that user code adds after anawaitand beforeresume(), which works on Bun today.req.close()orreq.destroy()(RST_STREAM with NO_ERROR), node emitsaborted, finish, end, closeon the paused server stream. Bun emitsaborted, finish, close. The stream is destroyed, only'end'is missing. That path does not go through state 7.duplexPair, an untouched server stream with an unread POST body that later callswrite()thenend(cb)loses'finish', andcbgetsERR_STREAM_DESTROYED. Node givesend, finish, close. Over TCP, Bun is correct. The cause is the destroy at'end'indestroySelfOnEndwhile a write callback is still deferred. A fix there has to handle a stream that errors while it waits, which node:http2: emit 'close' on a stream errored by a write() after end() #43346 works on.close(code)on a paused stream. Bun's event sequence differs from node's in 6 of 16 measured rows (codes 0, 8, 2, 11, with and withoutrespond(), in the listener or after a timer). For exampleclose(8)without a response: nodeaborted, finish, close, Bun adds'end'. main and this PR print the same sequence in all 16 rows.pushStream()never reaches state 7 on main, paused or not (node:http2: release server push streams once their response ends #38082 is open for that).Self-review. An independent review of this diff ended with "merge after changes, keep the code shape". It asked for three changes to this text, all made: name the regressing change and the last good release, say that no user reported it and give the measured reach, and list the JS socket row under "Not covered". I checked each of its claims on my own runs first. Two did not hold up and are not repeated here:
pipe()into a destination that errors is not affected on main, and 1.3.14 does not lose the unread bytes (it destroys the stream before the reader, see Provenance). The review kept one more concern about the implementation that I could not recover in full. So I re-ran its last probes (half close, reset from the peer, reach). They showed two claims of my earlier text as too broad, and the first two items of "Not covered" correct them.Suites on the debug build, this commit.
test/js/node/http2/(11 files,node-http2.test.js396 pass),test/js/node/test/{parallel,sequential}/test-http2-*.js(261 pass),fetch-http2-client.test.ts. Before the rebase alsofetch-backpressure.test.ts,node-http2-ping-flood-staged.test.ts, the http2 regression tests (25589, 24924, 26915, 29073), andtest/js/third_party/grpc-js/: 322 pass, 6 fail (five DNS lookups of public hosts with no network here, andtest tonic server). I did not run grpc-js without the change. #43433 reports the same failures on a debug build of main.[human-review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file