Repository navigation
Conversation
…e dropped A stream that closes on the wire while its data is unread stays undestroyed until the data is read. The native side closes the stream first and drops its id at the next socket read. Http2Stream#state still called the native getStreamState(), which throws "Invalid stream id" for an unknown id. util.inspect(stream) reads state, so it threw too. The getter now reports what node reports once nghttp2 has dropped a stream: state 1 (idle) and zeros. It does so from the moment the native side reports the full close, because node never shows the closed state. getStreamState() returns undefined for an unknown id, and the getter maps that to the same idle object. A pushed client stream, which the native stream table does not hold, no longer throws either. The NativeClosed bit is now set after the close channel publishes. A subscriber of http2.*.stream.close still reads the live state there, as in node.
|
Status: ready for review (#43801) How to reproduce on bun 1.4.3 (node v26.3.0 prints the state object and import http2 from "node:http2";
import util from "node:util";
const server = http2.createServer();
server.on("stream", s => { s.respond({ ":status": 200 }); s.end(Buffer.alloc(100, 0x61)); });
server.listen(0, "127.0.0.1", () => {
const client = http2.connect("http://127.0.0.1:" + server.address().port);
const req = client.request({ ":path": "/" });
const ping = () => new Promise(r => client.ping(() => r()));
req.on("response", async () => {
// The response is complete, but nothing reads it: closed on the wire, not destroyed.
await ping(); await ping();
const out = [`closed=${req.closed} destroyed=${req.destroyed}`];
try { out.push("state=" + JSON.stringify(req.state)); } catch (e) { out.push("state threw: " + e.message); }
try { util.inspect(req); out.push("inspect ok"); } catch (e) { out.push("inspect threw: " + e.message); }
console.log(out.join(" | "));
process.exit(0);
});
});Test: |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 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 (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. WalkthroughThe change centralizes HTTP/2 native stream-close handling, updates post-close state reporting, returns ChangesHTTP/2 stream state behavior
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The HTTP/2 close-state changes present no established merge-blocking risk. 🚥 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— Users still get an 'Invalid stream id' throw from stream.endAfterHeaders in exactly the closed-with-unread-data window this PR fixes for stream.state. The sibling getter at src/js/node/http2.ts:2552-2558 calls native getEndAfterHeaders, which still throws at h2_frame_parser.rs:4942 for an evicted id, and pushed client streams hit the same throw for the stream's whole life. Fix: apply the same treatment to every native per-stream getter in the file (return a JS-side value once NativeClosed is set, and have the native side return undefined for an unknown id), rather than fixing only the getter util.inspect happens to read.Why this was flagged
Trigger: a stream that closed on the wire while its data is unread (the PR's own repro: server responds and the client never reads; after the next socket read the native table drops the id at h2_frame_parser.rs:3618). Reading stream.endAfterHeaders at src/js/node/http2.ts:2555 then calls getEndAfterHeaders, whose lookup at h2_frame_parser.rs:4940-4946 still returns Err(global_object.throw("Invalid stream id")). The same throw occurs for any open pushed client stream because promised ids are never in the table. Base behaves identically, but the PR establishes the invariant that a JS stream outlives its native entry and fixes one of two getters that read the table, leaving the sibling with the exact throw the PR title promises to remove. Population: any code reading endAfterHeaders after a wire close (node returns a boolean there). Remedy: make getEndAfterHeaders total the same way (return undefined -> false), or cache endAfterHeaders as a JS flag at headers time.
Verification: pre-existing; acknowledged in diff: the PR description's "Downsides" says "
stream.endAfterHeadersstill throws in the same window. #41945 replaces that getter with a JS flag" — accurate for the closed-with-unread-data window, but it does not mention that pushed client streams hit the same throw for their whole life. Trigger: readingstream.endAfterHeaderson a stream whose native table… -
🟣
src/js/node/http2.ts— Server apps that call sendTrailers() and then read stream.state or util.inspect(stream) in the same tick see state 7 with localClose 1 and remoteClose 1, a value node never reports. The streamEnd handler at src/js/node/http2.ts:4116-4122 (client twin at :5120-5126) re-queues the state 7 dispatch on process.nextTick when kSendingTrailers is set, but the native entry is already CLOSED, and NativeClosed is not set until the deferred handler runs. The getter at http2.ts:2515-2519 therefore returns the raw native closed state in that tick. Fix: mark the JS stream natively closed (or return the idle object) whenever the native lookup reports the closed state, so the state 7 window is hidden on every deferral path, not only after the streamEnd handler has run.Why this was flagged
Trigger: ServerHttp2Stream.sendTrailers() at src/js/node/http2.ts:2464-2475 on a stream whose remote side is already closed; the native submit fully closes the stream synchronously and dispatches streamEnd(7), which the handler at :4116-4122 defers with process.nextTick because stream[kSendingTrailers] is true. Until that tick runs the native Stream is in StreamState::CLOSED and the JS status word lacks NativeClosed, so
get state()at http2.ts:2515-2519 calls native.getStreamState and returns {state: 7, localClose: 1, remoteClose: 1, ...}. The PR's stated contract (comment at http2.ts:2511-2514 and the PR text: 'node never reports the closed state') is violated on this path; the same happens on a Duplex transport where a peer write lands before the deferred handler, which the PR text itself lists among cases that only the native fallback covers. Population: any server using trailers (gRPC servers send trailers on every response). Remedy: have the getter map a native state of 7 to the idle object, or set the bit at the point the native close is first observed.Verification: pre-existing. Trigger: a server stream in HALF_CLOSED_REMOTE (ordinary GET whose request body ended) calls
sendTrailers()and then readsstream.state/util.inspect(stream)in the same tick. Mechanism verified in code: nativesend_trailersat /home/claude/bun/src/runtime/api/bun/h2_frame_parser.rs:5846-5856 synchronously does `stream.state = StreamState::CLOSED;… | pre-existing — the base…
A subscriber of http2.*.stream.close can destroy the session from inside the publish. destroyStreamForSessionDestroy() and _destroy() read the NativeClosed bit there, so the bit must be set first, as before. The state getter still has to read the live stream during that publish. A transient InNativeClose bit covers the publish, and the getter asks the native side while it is set.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…he publish NativeClosed stays where it was: set before the close channel publishes. A second bit, NativeCloseSettled, is set after markStreamClosed() returns, and only the state getter reads it. The getter needs no compound check, and markStreamNativeClosed() needs no try/finally. The idle state object moves into droppedStreamState().
|
Replies to the two findings outside the diff. Neither changes code in this PR.
|
There was a problem hiding this comment.
I re-reviewed the latest push and found no new bugs; the reordering in d4f100e (NativeClosed set before markStreamClosed, NativeCloseSettled after) resolves the session-destroy-from-subscriber path I raised earlier. Since this changes observable node:http2 semantics and shares the get state() getter with the in-flight #43754, a human look before merge would still be worthwhile.
What was reviewed:
getStreamStatethrow ->undefined: the getter at src/js/node/http2.ts:2526 is the only JS caller, and the only test asserting "Invalid stream id" (node-http2-streams-rehash.test.ts) targetsgetStreamContext, so nothing else depended on the throw.droppedStreamState()field values match node'sHttp2Stream::RefreshStatenull-stream branch (IDLE, all others 0); the{}results for missing native handle / no id / destroyed session are preserved.- The
NativeCloseSettledbit is consumed only by the getter and set only viamarkStreamNativeClosed, which both state==7 handlers (client and server) now use.
Extended reasoning...
The change touches src/js/node/http2.ts (a new StreamState bit, a markStreamNativeClosed helper, and a reworked Http2Stream#state getter), a one-line return change in src/runtime/api/bun/h2_frame_parser.rs, and five new tests in test/js/node/http2/node-http2.test.js. It touches no auth, crypto, or input-parsing surface; the Rust change only swaps a throw for an undefined return on a missing map entry. The source diff is small and the ordering concern from the previous review was addressed by a follow-up commit. Deferring rather than approving because the PR itself documents a remaining deviation from node for open pushed streams (delegated to another PR) and a getter overlap with #43754 that a maintainer should sequence, and because no debug build was available here to rerun the suite.
This branch has not been deployed
1 participant
Problem
stream.statethrowsInvalid stream idon a stream that closed on the wire while its data is unread.util.inspect(stream)readsstate, so it throws too. Node v26.3.0 returns state 1 (idle) and zeros.src/runtime/api/bun/h2_frame_parser.rs:3618).get state()(src/js/node/http2.ts) still calls nativegetStreamState(), which throws for an unknown id.Fix
get state()returns the idle object once the full close has reached JS. Until its next read the native side reports state 7, which node never shows.getStreamState()returnsundefinedfor an unknown id. The getter maps that to the idle object, as node'sHttp2Stream::RefreshStatedoes (node_http2.cc#L3161-L3167).NativeCloseSettled, set after thehttp2.*.stream.closepublish. A subscriber still reads live values, as in node.test/js/node/http2/node-http2.test.js(5 new tests, 3 fail on bun 1.4.3), all oftest/js/node/http2/, 27 vendored tests. Self-reviewed: the points raised are addressed (Notes).Background
'end'.NativeClosed, the existing bit for the full close, is still set before the publish. Session teardown reads it.Downsides
stream.endAfterHeadersstill throws in the same window, and on a pushed client stream. node:http2: make the stream lifecycle getters total, like node #41945 replaces that getter with a JS flag.Notes
Repro. The server responds with 200 and 100 bytes, the client never reads. After two
session.ping()round trips:The same happens on a server stream whose consumer is paused. bun 1.4.2 throws too. bun 1.3.14 does not: it destroyed the stream at the close.
Why the JS check is needed. A
setImmediatepoll ofreq.statefrom'response'shows the first value afterclosedturns true:The native entry stays in state 7 until the next socket read, which can be far away on an idle connection. A native fallback alone still shows state 7 in that window.
Why the native fallback is needed. The status bit is not set on every path that loses the native entry. Each of these threw
Invalid stream idwith the JS check alone, and now reports the idle object:'stream'and'push'time (the native stream table has no entry for a promised id),sendTrailers()on such a session, read after a synchronous peer write and before the deferred close handler.The close channel. A synchronous
http2.client.stream.closeorhttp2.server.stream.closesubscriber reads, for a plain request:NativeClosedis set before the publish, as on main, becausedestroyStreamForSessionDestroy()and_destroy()read it when a subscriber destroys the session or the stream. Ahttp2.server.stream.closesubscriber that callssession.destroy()still leaves a paused stream alive with its unread data, as on main (node destroys that stream). The getter keys onNativeCloseSettled, whichmarkStreamNativeClosed()sets aftermarkStreamClosed()returns, so it still asks the native side during the publish. No other code reads that bit. The test assertslocalCloseandremoteClose, which node and bun agree on.Tests. Each part has a test that fails without it. Without the JS check, the two closed-stream tests fail with state 7 at the first assertion. Without the native fallback, the pushed-stream test fails with
Invalid stream id. With the getter keyed onNativeClosed, the close-channel test fails. Theclose()test guards the getter against a check onclosed: right afterclose()the native stream is still live, and node and bun both read state 5. The closed-stream tests assert twice: in the first turn of the event loop that seesclosed(before the next socket read), and after two PING round trips. A stream can reach the full close in the same read as the first PING ACK, and the native side drops the id at the start of the next read, so the second ACK proves the drop on both sessions. The same scenarios pass on node v26.3.0 as a standalone script. 20 reruns of the five tests pass on the debug build.Self-review. The review's summary named these points. The first version of this text said that code which works on node sees no change, which is false for a close-channel subscriber. It said the deferred close handler always runs before the next read, which is false on a Duplex transport. Two throws remained with the bit unset (the pushed stream and the reset stream above). The native getter was not total. The window before the next read had no test. This diff and this text address each of them.
Not changed. In the same tick as
sendTrailers(), a stream whose remote side is already closed can read state 7. bun submits the trailers at once and defers only the JS close handling. Node defers the submission, and reads the live half-closed state there (state 6,localClose0,remoteClose1). Neither state 7 nor the idle object is node's answer in that tick, so this PR leaves that window alone.Related PRs. #43754 also guards
get state()onNativeClosedand returns{}. This PR returns node's object. The PR that lands second needs a small rebase of that getter. #43433 and #43754 use the twostreamEndlines that now callmarkStreamNativeClosed()as context.Found outside this task.
Http2Stream#_finalreads the status word, publisheshttp2.client.stream.bodySent, then writes the stale value back (src/js/node/http2.ts,this[bunHTTP2StreamStatus] = status | StreamState.FinalCalled). On a session over a Duplex transport, a subscriber that lets the peer finish makes the full close arrive inside the publish. The write-back then clearsClosedandNativeClosed, andstream.closedreadsfalseuntil the stream is destroyed. No open PR covers it. This PR does not change it.Suites run with the debug build.
test/js/node/http2/node-http2.test.js(395 pass, 6 skip), the other 10 files oftest/js/node/http2/(189 pass), the 18 vendoredtest-diagnostics-channel-http2-*.jsfiles, andtest-http2-session-stream-state,test-http2-stream-client,test-http2-server-stream-session-destroy,test-http2-client-set-priority,test-http2-client-setLocalWindowSize,test-http2-server-setLocalWindowSize,test-http2-no-more-streams,test-http2-respond-file-errors,test-http2-respond-file-fd-errors.[human-review] gate passed · iteration 0 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 0
evidence per changed file