Repository navigation
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. WalkthroughChangesThe HTTP/2 parser no longer manages HTTP/2 cancellation flow
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to No confirmed merge-blocking risk remains in the HTTP/2 cancellation and request-slot changes. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: merged with main (13a98b0), one more test block added. The diff is ready for review. All review threads are resolved. Two reports are reproduced here. Both are in 1. A caller gives its request up while the session is at the peer's limit. The client starts h2-destroy-at-limit.cjs// usage: bun h2-destroy-at-limit.cjs [how = destroy | destroy-error | timeout-destroy | pipeline | close | abort | none] [limit = 1] [runtime of the peer = node]
// (loopback only; the peer is a plain node:http2 server in a second process; it allows <limit> streams at a time: SETTINGS_MAX_CONCURRENT_STREAMS)
// A node:http2 client starts limit + 3 numbered requests on one session: <limit> go out, 3 wait for a free slot. 20 ms later the caller of request 1 gives it up.
// expected (node): RST_STREAM for request 1 goes out, THEN the next waiting request: everybody else gets its own answer: "RESULT: ok", exit 0.
// actual (bun, how = destroy ...): the waiting request's HEADERS go out BEFORE the RST_STREAM: the peer counts one stream too many and ends the whole session.
const http2 = require("node:http2"), { spawn } = require("node:child_process"), { pipeline, Readable } = require("node:stream");
const [how = "destroy", limitS = "1", peer = "node"] = process.argv.slice(2), limit = +limitS;
if (how === "server") {
const s = http2.createServer({ settings: { maxConcurrentStreams: limit } });
s.on("session", x => x.on("error", () => {}));
s.on("stream", (st, h) => { st.on("error", () => {}); st.resume(); setTimeout(() => { if (!st.destroyed) { st.respond({ ":status": 200 }); st.end("answer-" + h[":path"].slice(1)); } }, 60); });
s.listen(0, "127.0.0.1", () => console.log("PORT " + s.address().port));
} else {
const srv = spawn(peer, [__filename, "server", limitS], { stdio: ["ignore", "pipe", "ignore"] });
srv.stdout.once("data", d => {
const c = http2.connect("http://127.0.0.1:" + /PORT (\d+)/.exec(String(d))[1]), got = {}, N = limit + 3; let session = "alive";
c.on("error", e => (session = e.code)); c.on("goaway", code => (session = "GOAWAY " + code));
c.on("remoteSettings", () => {
for (let n = 1; n <= N; n++) {
const ac = new AbortController(), post = n === 1 && how === "pipeline";
const r = c.request({ ":path": "/" + n, ":method": post ? "POST" : "GET" }, n === 1 && how === "abort" ? { signal: ac.signal } : {}); let body = ""; got[n] = "never settled";
r.on("data", x => (body += x)); r.on("end", () => (got[n] = body || "no body")); r.on("error", e => (got[n] = e.code || e.message)); r.on("close", () => { if (got[n] === "never settled") got[n] = "closed"; });
if (post) pipeline(new Readable({ read() { setTimeout(() => this.destroy(new Error("source failed")), 20); } }), r, () => {}); else r.end();
if (n !== 1) continue;
if (how === "timeout-destroy") r.setTimeout(20, () => r.destroy());
else setTimeout(() => { if (how === "destroy") r.destroy(); else if (how === "destroy-error") r.destroy(new Error("gave up")); else if (how === "close") r.close(); else if (how === "abort") ac.abort(); }, 20);
}
});
setTimeout(() => {
const others = Object.keys(got).filter(n => n !== "1"), lost = others.filter(n => got[n] !== "answer-" + n), kinds = [...new Set(lost.map(n => got[n]))];
console.log(`how ${how}, limit ${limit}: request 1: ${got[1]}; the session: ${session}`);
console.log(`the other callers: ${others.length - lost.length} of ${others.length} got their own answer` + (lost.length ? "; the rest: " + kinds.join(", ") : ""));
console.log(lost.length ? "RESULT: one caller gave its request up and other callers lost theirs" : "RESULT: ok"); srv.kill("SIGKILL"); process.exit(lost.length ? 1 : 0);
}, 900);
});
}
On this branch each of the seven ways gives exit 0 in 3 of 3 runs. The script waits 900 ms in total. That is too short for limit 100 on a debug build, so the test file waits for conditions and covers the seven ways at limits 1 and 100. 2. A signal aborts const http2 = require("node:http2");
const net = require("node:net");
const frame = (t, fl, sid, p = Buffer.alloc(0)) => { const b = Buffer.alloc(9 + p.length); b.writeUIntBE(p.length, 0, 3); b[3] = t; b[4] = fl; b.writeUInt32BE(sid >>> 0, 5); p.copy(b, 9); return b; };
const PREFACE = Buffer.from("PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n");
let exhausted; const windowExhausted = new Promise(r => (exhausted = r));
const server = net.createServer(s => {
let buf = Buffer.alloc(0), pre = false, got = 0;
s.on("error", () => {});
s.on("data", d => {
buf = Buffer.concat([buf, d]);
if (!pre) { if (buf.length < PREFACE.length) return; pre = true; buf = buf.subarray(PREFACE.length); s.write(frame(4, 0, 0)); }
while (buf.length >= 9) {
const len = buf.readUIntBE(0, 3); if (buf.length < 9 + len) break;
const t = buf[3], fl = buf[4]; buf = buf.subarray(9 + len);
if (t === 4 && !(fl & 1)) s.write(frame(4, 1, 0));
if (t === 0 && (got += len) >= 65535) exhausted();
}
});
});
server.listen(0, "127.0.0.1", () => {
const session = http2.connect("http://127.0.0.1:" + server.address().port);
session.on("error", () => {});
session.once("remoteSettings", async () => {
const ac = new AbortController();
const stream = session.request({ ":path": "/u", ":method": "POST" }, { signal: ac.signal });
stream.on("error", () => {});
let drains = 0, accepted = 0;
const backpressured = !stream.write(Buffer.alloc(65535 + 32768, 0x41));
stream.on("drain", () => { drains++; for (let b = 32; b > 0 && stream.write(Buffer.alloc(16384)); b--) accepted++; });
await windowExhausted;
stream.on("close", () => setImmediate(() => { console.log(JSON.stringify({ backpressured, drains, accepted })); process.exit(0); }));
ac.abort();
});
});
|
|
Updated 7:40 PM PT - Oct 5th, 2026
✅ @robobun, your commit b386b070013547b90b14b1c87332a964b32a088e passed in 🧪 To try this PR locally: bunx bun-pr 42763That installs a local version of the PR into your bun-42763 --bun |
The native H2FrameParser also listened to the AbortSignal of a request. That listener ran before the JS abort listener. It closed the native stream and dropped the queued DATA frames, and this completed the held Writable write callback with success while the JS stream was still live. Writable then emitted 'drain'. Later writes reached the closed native stream and were reported as successful. Remove the native signal handling. The JS listener already destroys the stream with an AbortError, like node, and _destroy sends RST_STREAM(CANCEL). The stream is destroyed before the queued frames are dropped, so Writable skips 'drain' and rejects later writes. The native path also emitted 'aborted' for a stream whose writable side had ended, rejected a signal that is not a native AbortSignal, and ran before abort listeners that were added before request(). Node does none of these.
destroy() and close() send RST_STREAM from a setImmediate that is queued
before 'close'. The client released the SETTINGS_MAX_CONCURRENT_STREAMS
slot on 'close', so the HEADERS frame of the next queued request reached
the peer before the RST_STREAM. The peer still counted the first stream
as open and refused the new one with REFUSED_STREAM.
Release the slot one setImmediate after 'close'. The native signal
listener used to write RST_STREAM at once, which hid this for
request({ signal }). stream.destroy() had the problem before.
60c3241 to
02eec28
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…the connection ends The test helper's wireLength(n) and windowExhausted now reject when the peer socket closes or the session errors or closes, so a regression fails with the frames that were seen and does not wait for the timeout.
_destroy queues the RST_STREAM and then the slot release, in that order, with two setImmediate calls in one place. The session no longer needs a 'close' listener per request that relies on the order of two unrelated setImmediate calls.
There was a problem hiding this comment.
Code review found no new issues
No new issues were found in this update; 2 findings from earlier reviews are still open above.
Still open from earlier reviews (2):
- 🔴
src/js/node/http2.ts:2667—A throwing 'aborted' listener now leaks the concurrency slot, so requests queued behind SETTINGS_MAX_CONCURRENT_STREAMS… - 🔴
src/js/node/http2.ts:6295—A http2.client.stream.start diagnostics_channel subscriber that destroys the stream leaks a concurrency slot, so later…
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
The release from Http2Stream#_destroy lost the slot in two cases. A
listener that throws inside _destroy ('aborted') skipped the release. A
diagnostics_channel subscriber of http2.client.stream.start that destroys
the request ran _destroy before the request was counted.
'close' fires in both cases. The listener releases the slot one
setImmediate later, which is after the setImmediate that sends the
RST_STREAM. Add a test for the diagnostics_channel case.
There was a problem hiding this comment.
I reviewed the latest push and found no new issues. Moving the slot release back to a 'close' listener (cf8754c) addresses both open findings — 'close' is emitted from destroy.ts via nextTick even when _destroy throws from emit("aborted") and even when destroy() ran before #trackActiveRequest registered the listener — and the new diagnostics_channel test covers the second case. Given the scope (native abort path removed, RST_STREAM/slot-release ordering), a human look is still worthwhile.
What was reviewed
#trackActiveRequest: theonce("close", …)+setImmediaterelease fires on every destroy path, including a throw betweenemit("aborted")and the RST_STREAM scheduling.- Native deletions:
StreamState::IDLEhad no remaining producer after the pre-aborted branch was removed; thefinalizestream-free loop only existed to dropSignalRef's parser refs. - Test helper:
wireLength/windowExhaustednow reject with the observed frames when the connection ends first, so a regression fails instead of hanging.
Extended reasoning...
Overview
This PR removes the native AbortSignal handling for http2.connect().request({ signal }) from src/runtime/api/bun/h2_frame_parser.rs (~144 lines: SignalRef, attach_signal, abort_stream, StreamState::IDLE, the shutdown free loop) and relies on the existing JS abortRequestStream → stream.destroy(new AbortError()) path in src/js/node/http2.ts. To keep RST_STREAM on the wire before the next queued request's HEADERS, #trackActiveRequest now defers the concurrency-slot release by one setImmediate after 'close'. A 326-line dual-runtime test file exercises abort-during-flow-control-block, wire ordering under SETTINGS_MAX_CONCURRENT_STREAMS: 1, and non-native signal objects.
Security risks
None identified. The change removes native code rather than adding it, and the JS path already existed. No new user-controlled input reaches native. The test server binds port: 0 on localhost only.
Level of scrutiny
Moderate-to-high. This is node:* compat where event ordering and wire framing are contract, and the PR description names remaining wire-order differences from Node (GOAWAY/RST_STREAM ordering, corked DATA before RST_STREAM) that are shared with stream.destroy(err) on main. The native deletion also removes a VM-shutdown cleanup loop and a stream state variant, so lifecycle correctness needs a human eye. The two 🔴 findings from the earlier review were both slot-leak regressions introduced by an intermediate approach (release scheduled inside _destroy); cf8754c reverts to the base-branch shape ('close' listener) with only a setImmediate added, which restores the base-branch guarantee that the release survives any throw inside _destroy and any pre-registration destroy().
Other factors
The PR author added a targeted test for the diagnostics_channel case raised earlier, and the test helper now rejects its waiters on connection close (the earlier optional finding). The PR description reports the full test/js/node/http2/ suite and 261 vendored test-http2-*.js passing on debug+ASAN, plus 30/30 runs under BUN_DESTRUCT_VM_ON_EXIT=1 with leak detection. The remaining wire-order differences are explicitly documented as pre-existing and shared with the non-signal destroy() path.
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
setImmediate is not the precise indicator of completion here.
|
Agreed. The second The exact signal exists already: the native side reports a closed stream through the |
…closed A setImmediate after 'close' only happened to run after the one that sends the RST_STREAM. The exact signal is the native close notification: the streamError, aborted and streamEnd (state 7) handlers, which already decrement #connections. Release the SETTINGS_MAX_CONCURRENT_STREAMS slot there and submit the queued requests. nghttp2 counts the same way. end_stream dispatched before it wrote the RST_STREAM frame. It now writes the frame first, like the engine's own resets, so a request that the handler submits follows the RST_STREAM on the wire. The slot is taken before the native request() call, because that call can report the stream closed. A request whose response ended no longer holds its slot until something reads the body.
|
Done in 041e681.
One more case now matches node: a request whose response ended frees its slot at once, even if nothing reads the body. On main the queued request waits until the body is consumed. The test file has a case for it (7 tests, main fails 6, node v26.3.0 passes 7). All of |
… runs Http2Stream#_destroy computed the code and queued the RST_STREAM after it emitted 'aborted'. A listener that throws stopped _destroy there, so the RST_STREAM was never sent: the native stream stayed open, the peer kept counting it, and the session could not finish a graceful close. With the slot release tied to the native close, a queued request also waited forever. Set the code and queue the RST_STREAM first, like node's closeStream(). stream.rstCode now has its final value inside 'aborted' listeners, as in node.
|
This PR also fixes a second report. A response that ended, but that nothing reads, keeps its Repro with a real const http2 = require("node:http2");
const server = http2.createServer({ settings: { maxConcurrentStreams: 2 } });
server.on("stream", stream => {
stream.on("error", () => {});
stream.resume();
stream.on("end", () => {
stream.respond({ ":status": 200 });
stream.end("response-body");
});
});
server.listen(0, () => {
const client = http2.connect(`http://127.0.0.1:${server.address().port}`);
client.on("error", e => console.log("client session error", e.code));
client.on("remoteSettings", async () => {
for (let i = 0; i < 2; i++) {
const req = client.request({ ":path": "/", ":method": "POST" });
req.on("error", () => {});
req.end("body"); // the response is never read
}
await new Promise(r => setTimeout(r, 800));
const third = client.request({ ":path": "/third", ":method": "POST" });
let thirdStatus = null;
third.on("response", h => (thirdStatus = h[":status"]));
third.on("error", () => {});
third.resume();
third.end("x");
await new Promise(r => setTimeout(r, 800));
console.log(JSON.stringify({ thirdPending: third.pending, thirdStatus }));
process.exit(0);
});
});Output, debug builds:
The branch now conflicts with main in |
One caller gives its open request up while other requests wait for a slot. The peer is node's own http2 server, which ends the session when a client exceeds its SETTINGS_MAX_CONCURRENT_STREAMS. The request is given up in seven ways, at a limit of 1 and of 100. Each other caller must get its own answer. Also checks the wire order for a plain destroy().
robobun
changed the title
| setting(SETTINGS_MAX_CONCURRENT_STREAMS, 1), | ||
| { respond: true }, | ||
| ); |
There was a problem hiding this comment.
🟡 nit (optional): On a CI lane without a node binary, all 14 peer-limit tests in this file now fail instead of being skipped. before throws "node executable not found" at test/js/node/http2/node-http2-request-signal.test.ts:429 when Bun.which("node") returns null. The sibling node-http2-client-close.test.ts:334 skips its node spawn when node is missing and on musl, where node segfaults and the runner fails the file for the core dump. Fix: gate the node-peer describe with skip when node is not found (and on musl like the sibling), so the first two describes still run everywhere.
Why this was flagged
The third describe spawns the system node as the HTTP/2 peer. In before at test/js/node/http2/node-http2-request-signal.test.ts:363-365, Bun.which("node") returning null makes the hook throw new Error("node executable not found"), which fails every limit ${limit}, ${how} test (2 limits x 7 ways = 14 tests) rather than skipping them. REVIEW.md asks tests needing a system binary to skipIf when it is unavailable. The existing test/js/node/http2/node-http2-client-close.test.ts:334 does exactly that ({ skip: !node || isMusl }) and also documents that Alpine's node segfaults and the CI runner fails a file for any new core dump; this new file spawns node on the musl lane unconditionally. On the base branch this file does not exist, so no http2 test fails for a missing node binary.
Verification: nit — triggers when Bun.which("node") returns null. test/js/node/http2/node-http2-request-signal.test.ts:428-429 throws "node executable not found" inside the before hook of the third describe, so a missing node makes every limit ${limit}, ${how} test (14 tests) fail rather than skip. The sibling node-http2-client-close.test.ts:325-334 does gate its node spawn with { skip: !node || isMusl }.
| req[bunHTTP2StreamStatus] &= ~StreamState.HoldsRequestSlot; | ||
| this.#activeRequestCount--; |
There was a problem hiding this comment.
🟡 nit (optional): The flush-path catch at src/js/node/http2.ts:6371-6372 re-implements #releaseRequestSlot without its flag guard, so a native request() that dispatches onStreamError and then throws decrements #activeRequestCount twice. The count then goes below the real number of open streams and the client submits more requests than the peer's SETTINGS_MAX_CONCURRENT_STREAMS allows, which the peer answers with REFUSED_STREAM. Fix: release the slot through one guarded helper at every site (the HoldsRequestSlot check must run before the decrement), e.g. split #releaseRequestSlot into a guarded decrement and the flush so the loop can call the decrement without re-entering itself.
Why this was flagged
Native request() in src/runtime/api/bun/h2_frame_parser.rs:6944-6968 dispatches onStreamError at 6948 when over the session memory limit, and only after that can fail with ? at 6959 (binary_type.to_js(...)). The JS streamError handler at src/js/node/http2.ts:5074 calls #releaseRequestSlot, which clears HoldsRequestSlot and decrements #activeRequestCount (6319-6322). When the same native call then throws, the #flushPendingRequests catch at 6371-6372 clears the flag again and decrements #activeRequestCount unconditionally, while the immediate-path catch at 6302 goes through the guarded #releaseRequestSlot. The counter is now one below the number of streams the peer counts, so the gate at 6343 lets one extra request through and the peer refuses it. On the base branch the slot was released once by the 'close' listener, so no double decrement was possible. The trigger is an allocation failure in that native path, so it is rare; the duplicated two-line block is the same logic as the helper minus its guard and should share it.
Verification: src/js/node/http2.ts:6370-6372 decrements #activeRequestCount unconditionally, unlike #releaseRequestSlot which returns early when the flag is already clear. The streamError handler (:5071-5076) calls #releaseRequestSlot, so if native request() dispatches onStreamError and then returns Err, the count is decremented twice. The only trigger is JSC OOM at h2_frame_parser.rs:6959.
Problem
SETTINGS_MAX_CONCURRENT_STREAMS, with requests queued for a free slot, and destroys an open request. The next HEADERS go out before that RST_STREAM. A node server answers GOAWAY, and each other request fails (ERR_HTTP2_GOAWAY_SESSION,ERR_HTTP2_STREAM_ERROR)._destroy(src/js/node/http2.ts) sends the RST_STREAM from asetImmediate. The slot was released on'close', one loop turn earlier.request(headers, { signal }): an abort while a write waits for window emits'drain'. The native listener (SignalRef,src/runtime/api/bun/h2_frame_parser.rs) called the queued write's callback with no error.Fix
streamError,abortedandstreamEnd(state 7) handlers.end_streamwrites the RST_STREAM frame before it dispatches._destroyqueues the RST_STREAM before any listener runs.abortRequestStreamalready destroys the stream, as node does.test/js/node/http2/node-http2-request-signal.test.ts(23 tests, main fails 16). Also all oftest/js/node/http2/and the vendoredtest-http2-*.js.Background
setImmediateafter'close': it only happens to run after the RST_STREAM. The native report is exact.Downsides
destroy()does.request(),write()andabort()in one tick send HEADERS, DATA, RST_STREAM. Main sent HEADERS, RST_STREAM.Notes
A request given up at the limit, against node's own server. The client starts
limit + 3requests on one session:limitgo out and 3 wait for a slot. The caller of request 1 then gives it up. The peer is a node v26.3.0http2.createServer({ settings: { maxConcurrentStreams: limit } })in a second process. nghttp2 ends the session with GOAWAY when a HEADERS frame exceeds the limit.destroy()destroy(err)setTimeout(ms, () => destroy())pipeline()with a source that failsclose()"main" is bun 1.4.3-canary.1 (367d939, a release build) and a debug build of 13a98b0. Both give the same result at limit 1 and at limit 100. Against a raw peer, main writes
HEADERS 1, HEADERS 3, RST_STREAM 1fordestroy(). This PR and node writeHEADERS 1, RST_STREAM 1, HEADERS 3. The test file has the 14 cases (7 ways, limits 1 and 100). They wait for conditions and not for time: the peer holds the firstlimitanswers until the client resets one of them.Merged with main at 13a98b0. One file conflicted,
h2_frame_parser.rs. #42682 removedis_stream_aborted. #43933 moved the stream's native signal listener toAbortHandle(SignalRef,attach_signal,abort_stream_for_signal). This PR removes that listener, so the merge deletes those three.tsc --noEmit -p src/js/tsconfig.jsonpasses. The merge and the new tests did not go through another self-review round, because the source diff against main is the same change as before.Cost per request, from the diff. One closure and one
'close'listener fewer (#trackActiveRequest). A{ signal }request has oneBox<SignalRef>, one parser ref and oneAbortHandleregistration fewer. Each of the three native close handlers has one more call with a flag test.test-http2-forget-closed-streams.js(10000 requests in a row) on the debug build, run alone: main 94 s and 116 s, this PR 90 s, 96 s and 100 s.perfandvalgrindare not installed in the container that ran this, so there is no instruction count.This is the
request({ signal })path that #42717 names as excluded. That PR fixes the same'drain'bug for the session teardown paths and does not touch this code.Other node mismatches of the native path. The same deletion fixes them, and the new test covers the first two:
stream.end(body), then an abort while the body is still blocked: main emits'aborted'and setsstream.aborted. Native looked at its own stream state (OPEN, END_STREAM not sent yet). Node and_destroylook atwritableState.ending.AbortSignal(any event target with anabortedproperty): main throwsERR_INVALID_ARG_TYPEfrom nativerequest(). Node accepts it (validateAbortSignalplusaddAbortListener).abortlistener that was added to the signal beforerequest()sawstream.aborted === true. In node it sees a live stream.Removed with it.
StreamState::IDLEwas constructed only in the native pre-aborted branch, which JS never reached (JS handles a pre-aborted signal before it calls native).H2FrameParser::finalizefreed the streams early at VM shutdown only to release the parser refs thatSignalRefheld. AStreamholds no parser ref now, soDropfrees the streams once the refcount reaches zero.process.exit()from inside a stream'data'callback with open streams, withBUN_DESTRUCT_VM_ON_EXIT=1anddetect_leaks=1: no leak report.Checked against node v26.3.0 with a raw h2c peer, after the change. RST_STREAM(CANCEL) reaches the wire,
'aborted'fires once,rstCodeis 8, the'error'is anAbortErrorwhosecauseis the signal reason, the abort listener count on the signal returns to 0 after'close', an abort before connect sends nothing, two streams on one signal each send their RST_STREAM,write()after the abort returnsfalsewithERR_STREAM_DESTROYED, abort followed bysession.destroy()sends only GOAWAY.Wire differences that remain, all shared with
stream.destroy(err)on main. Withabort(); session.close()in one tick the order is GOAWAY, RST_STREAM (node: RST_STREAM, GOAWAY). Withrequest(); write(); abort()in one tick the corked chunk is flushed, so the peer sees HEADERS, DATA, RST_STREAM. Main's signal path sent HEADERS, RST_STREAM. Node sends nothing, because nghttp2 cancels the HEADERS that it has not sent yet.The slot release,
{status of the queued request}withmaxConcurrentStreams: 1(node gives 200 in every row):destroy(err)close(NGHTTP2_CANCEL)The same holds when user code sends the next request from the
'close','error'or'aborted'listener of the cancelled one.Where the slot is released. nghttp2 frees the slot when the stream closes on the wire. The native side reports exactly that through three handlers (
streamError,aborted,streamEndwith state 7), which is where#connectionsis decremented. The release and the flush of queued requests happen there now, and the'close'listener per request is gone.end_stream(the JSrstStream()call) used to dispatch before it wrote the RST_STREAM frame. The engine's own resets write first, andend_streamdoes the same now, so a request that the handler submits follows the RST_STREAM. The slot is taken before the nativerequest()call, because that call can report the stream closed (invalid options, memory limit), and it is given back if the call throws. An earlier version of this PR released the slot onesetImmediateafter'close', which only happened to run after the one that sends the RST_STREAM (review comment by @Jarred-Sumner).One effect of the exact signal, the same in node v26.3.0: a request whose response ended frees its slot at once, even if nothing reads the body. On main the queued request waits until the body is consumed (covered by the test).
_destroyqueues the RST_STREAM first._destroycomputed the code and queued the RST_STREAM after it emitted'aborted'. A listener that throws stopped it there (destroy.ts catches the throw and still emits'error'and'close'), so the stream was never reset: the native stream stayed open, the peer kept counting it,#connectionsstayed up and a gracefulsession.close()could not finish. This is the same on main for everystream.destroy(). With the release tied to the native close, a queued request would also wait forever (found in review)._destroynow sets the code and queues the RST_STREAM before any listener runs, like node'scloseStream().stream.rstCodetherefore has its final value inside'aborted'listeners, as in node: 8, 2 and 0 for a signal abort,destroy(err)anddestroy(). On main it is 8 for the signal path (set by the native listener) and undefined for the other two. Node v26.3.0 itself does not send the RST_STREAM when an'aborted'listener throws, and never submits the queued request. The test for this case is skipped under node.Not fixed here. A peer RST_STREAM for a stream with a flow-control-blocked write, on a session whose transport is a JS
Duplex(createConnectionon the client,server.emit('connection', duplex)on the server), also emits'drain': drains 1, accepted 32, node 0 and 0. On a native socket it does not, because the tick queue runs between the'aborted'/streamErrordispatch andclean_queue. Thereon_stream_resetdispatches first and the JS handler defers the destroy withprocess.nextTick, so it needs its own change.The test file. 23 tests. A debug build of main (13a98b0) fails 16. The 7 that pass there: the signal abort with a queued request against the raw peer (the native listener wrote the RST_STREAM at once), and
close(), the signal abort and the control against the node peer at both limits. Node v26.3.0 passes 22 and skips the throwing'aborted'listener case.Suites on the debug+ASAN build, after the merge. The 12 files in
test/js/node/http2/: 619 pass, 6 skip, 0 fail. The vendoredtest-http2-*.jsthroughscripts/runner.node.ts: 262 of 263 pass.test-http2-forget-closed-streams.jsran past the runner's 180 s limit while the other tests ran beside it. Run alone it passes, on main and on this PR (the times are above). grpc-jstest-retry,test-deadline,test-server-errors,test-call-propagationandtest-idle-timerpass. The test file also passes withBUN_DESTRUCT_VM_ON_EXIT=1,detect_leaks=1andBUN_JSC_validateExceptionChecks=1. It passes 12 of 12 runs on the debug build, 18 of 18 with six runs at once, and 20 of 20 on node.Self-reviewed: 6 concerns raised, 5 addressed (the RST_STREAM order for queued requests, test waits that could hang, assertion order, peer sockets left open, the shutdown path), 1 named above as a remaining wire difference. The test helper also rejects its waits when the connection ends first, so a regression fails with the frames that the peer saw.
[human-review] gate passed · iteration 3 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 0 rejected · iteration 3
evidence per changed file