Repository navigation
Conversation
…s request() caller A request made before the session connects, or while the peer's SETTINGS_MAX_CONCURRENT_STREAMS leaves no stream slot, waits in the session's pending queue. The flush submitted it in the async context of whatever drained the queue: the connect, a SETTINGS frame, or the 'close' of another request. Its write and end callbacks, 'ready', 'finish', the start and body diagnostics channels, its header value coercion and its failure events ran in that context. The flush now submits each request in the frame that the stream captured in request(), as node binds requestOnConnect to the request's async resource. The close()-while-connecting rejection does the same. A request of a disposed Bun.ModuleGraph stays in the drainer's context, and the RST_STREAM of a close() on a pending request is armed as the session: dispose() drops what was armed in the graph, and the stream would keep its slot.
|
Status: ready for review. Reproduced with the script in the PR description on main 5900354 (release build and debug ASAN build) and on Bun 1.4.2:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughHTTP/2 callback scheduling, stream resets, request rejections, and queued submissions now use captured async-context frames. The change adds a queued-submission helper and tests for context preservation, submission failures, cancellation, uploads, and module-graph disposal. ChangesHTTP/2 async-context handling
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
There was a problem hiding this comment.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🔴
src/js/node/http2.ts— Hosts sharing one http2 session over a JS Duplex transport can lose a stream slot for good when a Bun.ModuleGraph is disposed mid-upload; on main that waited upload still finished. After this change 'ready' is queued in the request's own frame (http2.ts:6414), so the write completion thatsession[kDeferWriteCallback]arms with setImmediate at http2.ts:2957 belongs to the request's graph and dispose() drops it. END_STREAM is never sent, 'close' never fires and #activeRequestCount stays raised, so other tenants' requests queue forever once the peer's limit is hit. Fix: work that frees the shared session's slot must survive the request graph's dispose, as sendRstOnReady does at http2.ts:2257. [also at: src/js/node/http2.ts:6373 - A host sharing an http2 session with a Bun.ModuleGraph over a JS Duplex transport can lose a stream slot for good when the graph is disposed mid-upload.]Why this was flagged
A host creates one ClientHttp2Session with createConnection returning a JS Duplex (so deferWriteCallbackForSocket at http2.ts:2083 returns setImmediate). A tenant Bun.ModuleGraph calls client.request() while the peer's maxConcurrentStreams leaves no slot, writes a body chunk and end()s; the host disposes the graph after the slot frees. On main the queued request was submitted in the drainer's frame, so the write immediate survived. Now process.nextTick(emitEventNT, req, "ready") at :6414 captures the tenant's frame; _write at :2934 continues on 'ready' in that frame and sessionkDeferWriteCallback at :2957 arms a setImmediate whose AsyncContextFrame is the disposed graph's; bindings.cpp:3305 drops it. The Writable callback never runs, so _final is never reached, no END_STREAM goes out and the once('close') from #trackActiveRequest at :6337 never decrements #activeRequestCount. The slot is taken until the session dies; every later request of the host and other tenants waits in #pendingRequests forever. The stopped-graph guard at :6368 only covers a graph disposed before submission, not one disposed during the upload.
Verification: Line 6414
process.nextTick(emitEventNT, req, "ready")captures the request's frame. At http2.ts:2957session[kDeferWriteCallback](callback)stays the class defaultsetImmediate. jsc_hooks.rs:1652-1673: disposing the graph cancels every tracked timer, so the write callback is never invoked. On the base branch the immediate belonged to the host's context and survived the tenant's dispose(). -
🟣
src/js/node/http2.ts— pre-existing: a Bun.ModuleGraph that calls close() on an already-submitted stream and is then disposed in the same tick never sends its RST_STREAM, so the stream keeps its slot. src/js/node/http2.ts:2601 arms setImmediate(rstNextTick...) in the close() caller's frame, and dispose() drops a disposed graph's immediates. The PR fixes exactly this for the parked (pending) sibling in sendRstOnReady at src/js/node/http2.ts:2257 but leaves the open-stream branch as the base. Fix: arm every RST_STREAM immediate of close() as the session, e.g. a shared helper used by both src/js/node/http2.ts:2257 and src/js/node/http2.ts:2601, so a stream's slot is freed whichever graph closed it.Why this was flagged
A stream was submitted (has an id) from inside a Bun.ModuleGraph. That graph calls req.close() and then graph.dispose() before the next immediate runs, for example in a teardown routine. close() takes the branch at src/js/node/http2.ts:2600-2601 and calls setImmediate(rstNextTick.bind(session, this.#id, code)) with the graph's frame active. dispose() drops immediates armed as that graph (the reason the diff gives at src/js/node/http2.ts:2256), so rstNextTick at src/js/node/http2.ts:2235 never calls rstStream and the peer never sees the RST_STREAM. The stream stays open at the peer and counts against SETTINGS_MAX_CONCURRENT_STREAMS, so a later host request waits for a slot the peer never frees. The base branch behaves the same on this line; the diff fixes only the pending-stream sibling at src/js/node/http2.ts:2257 by arming via runInFrame(session[bunHTTP2AsyncContextFrame], setImmediate, ...), and the open-stream branch two lines below sendRstOnReady's caller is not given the same treatment.
Verification: pre-existing — the base already fails by the same route; the diff fixes only the pending-stream sibling. Trigger: a Bun.ModuleGraph calls
close()on a stream that already has an id, and the graph is disposed before the next immediate runs. src/js/node/http2.ts:2601 arms the RST_STREAM immediate in the close() caller's frame, sorstNextTicknever callsrstStreamand the slot is held. -
🟣
src/js/node/http2.ts— pre-existing: a caller whose queued request is refused by a peer GOAWAY still gets its 'error' and 'close' in the GOAWAY dispatch's store, not its request() store. The PR frames the pending-request rejection only for close()-while-connecting (http2.ts:5446); the peer-GOAWAY loop at http2.ts:5340 calls streamRejectedByGoawaySession bare, as do the forEachStream rejections at http2.ts:5333 and http2.ts:5449. Fix: run the destroy inside stream[bunHTTP2AsyncContextFrame] in streamRejectedByGoawaySession itself, which covers the 4 sites and lets the wrapper at 5446 go. Same pattern at 4 sites (http2.ts:5333, http2.ts:5340, http2.ts:5446, http2.ts:5449). [also at: src/js/node/http2.ts:5446 - pre-existing: a queued request refused by a GOAWAY the peer sends still gets its 'error' and 'close' in the wrong AsyncLocalStorage store, unlike Node.]Why this was flagged
A client on a shared session has requests queued behind the peer's SETTINGS_MAX_CONCURRENT_STREAMS, each made inside als.run(user, ...). The peer sends GOAWAY. The native goaway handler at src/js/node/http2.ts:5320 runs in the parser dispatch context, and at http2.ts:5338-5342 it calls streamRejectedByGoawaySession(pendingRequests[i].req) with no runInFrame, so stream.destroy(err) at http2.ts:4975 and the nextTick'd 'error'/'close' run in that dispatch context, not the request() caller's frame. Node runs the equivalent refusal through the stream's AsyncWrap, so 'error'/'close' see the request() caller's store. The PR fixed the identical loop in #onConnect at http2.ts:5446 with runInFrame(req[bunHTTP2AsyncContextFrame], ...) but left this sibling; the forEachStream rejections at http2.ts:5333 and http2.ts:5449 call the same helper unframed for submitted streams. The base branch behaves the same way at these three sites, so this is pre-existing.
Verification: pre-existing. The PR frames the close()-while-connecting rejection (line 5446), but the byte-identical loop in the native goaway handler is untouched: src/js/node/http2.ts:5338-5342 calls streamRejectedByGoawaySession bare. No test exercises a peer GOAWAY against queued requests. The base branch already fails this way by the same route (the 5340 loop is unchanged).
-
🟣
src/js/node/http2.ts— A host that close()s an http2 session shared with a Bun.ModuleGraph can never see the session destroyed or 'close' when the last stream was a tenant's and that tenant is disposed first. The destroy immediate at src/js/node/http2.ts:5150 is armed inside streamEnd, which withStreamFrame runs under the finishing stream's frame; dispose() drops immediates of a stopped graph. This is the same class the PR fixes at src/js/node/http2.ts:2257 by arming the RST as the session, but the sibling site is left as is. Fix: arm every session-lifetime immediate (5150 and scheduleSettingsAckGraceNT at 5147) under session[bunHTTP2AsyncContextFrame], as sendRstOnReady now does.Why this was flagged
Trigger: a host session, tenant graph B's stream is the last active one, the host calls session.close(), B's stream finishes (native streamEnd state 7), then B is disposed before the immediate phase. streamEnd is wrapped by withStreamFrame (src/js/node/http2.ts:3001-3002) so it runs under B's frame; setImmediate(destroyIfNotDestroyedNT, self) at src/js/node/http2.ts:5150 is therefore an AsyncContextFrame of B, and AsyncContextFrame::call drops it once B's graph is stopped (src/jsc/bindings/AsyncContextFrame.cpp:159). The host's close() never completes: no session 'close', the socket stays open, the process keeps running. The base branch behaves the same, but the PR explicitly fixes this exact mechanism for the parked RST_STREAM at src/js/node/http2.ts:2257 with the comment that dispose() drops an immediate armed as the request's graph, and leaves this sibling in the same function family untouched; REVIEW.md asks for the whole class in one PR. Remedy: run the setImmediate at 5150 (and 5147) via runInFrame(self[bunHTTP2AsyncContextFrame], ...).
Verification: pre-existing. Trigger: a host closes a ClientHttp2Session whose last active stream was made by a Bun.ModuleGraph tenant, that stream reaches native state 7, and the tenant graph is disposed before the immediate phase. The session's only teardown is
setImmediate(destroyIfNotDestroyedNT, self)at src/js/node/http2.ts:5150, inside streamEnd understream[bunHTTP2AsyncContextFrame]; it never emits 'close'.
…2-queued-request-async-context
dispose() of a Bun.ModuleGraph drops the immediates armed in it. A write over a JS transport completes on an immediate, so a graph that was disposed in the middle of an upload left its stream open, and the stream kept a stream slot of the peer. The session arms that immediate now and runs the callback in the async context of the writer. The stopped-graph guard in the flush is removed. A request of a disposed graph is submitted in its own context, and its failure goes to the onError of its own graph.
|
Answers to the additional review findings, after 0d6ec09. A graph disposed in the middle of an upload over a The guard for a disposed graph. Removed, see the review thread.
A GOAWAY that rejects queued requests. Not changed. I measured node v26.3.0 with three requests at a stream limit of 1 and a GOAWAY with code 2 and last stream id 1. The second request gets The session destroy immediate. I could not reproduce a hang. With the tenant disposed at its stream's |
…2-queued-request-async-context # Conflicts: # src/js/node/http2.ts
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.
The second push drops the stopped-graph guard in the flush, so a disposed graph's queued request now submits in its own frame; that resolves what I raised on the first version. Beyond the inline finding, I also checked the new deferWriteCallbackAsSession for a wrong this: every call site invokes it as session[kDeferWriteCallback](callback), so the session frame it reads is always the right one, and native sockets still take the process.nextTick branch. isFrameOfStoppedModuleGraph has no remaining caller in http2.ts and its shared.ts export change is net-zero against base.
Extended reasoning...
The change touches src/js/node/http2.ts (pending-request flush, GOAWAY rejection, parked RST_STREAM arming, JS-transport write-callback deferral) plus a new node:test file; no auth, crypto, or input-parsing surface. The inline finding is pre-existing and the diff is otherwise consistent with the described Node behavior, but async-context framing in a compat module warrants a human look.
|
Merged main (9b4284f) to resolve the conflict. One hunk conflicted, in
The merge keeps both. Verified on the merge with the debug ASAN build:
The review of 0d6ec09 has no blocking finding. Its inline finding is the behaviour on main: the callback of a |
…2-queued-request-async-context # Conflicts: # src/js/node/http2.ts
…2-queued-request-async-context
Problem
http2.connect()session, a request that waited for a stream slot or the connect runs itswrite()/end()callbacks,'ready'and'finish'in another caller'sAsyncLocalStoragestore. Node v26.3.0 does not. No user reported it.#flushPendingRequests(src/js/node/http2.ts:6376) submits a queued request in the async context of what drained the queue (new in 1.4.0, node:http2: rewritten inbound engine, batched write path, server push, +290 node v26.3.0 tests (79% passing) #31584).Fix
request()(node:reqAsync.bind(requestOnConnect)).close()while connecting rejects queued requests that way too.close()and the write completion of a JS transport itself, soBun.ModuleGraph.dispose()cannot drop them.test/js/node/http2/node-http2-pending-request-async-context.test.ts(main fails 15 of 23, node passes its 11). Self-reviewed: 31 concerns raised, 24 addressed, 7 answered in Notes.Background
ClientHttp2Sessionqueues a request while the socket connects or the peer'sSETTINGS_MAX_CONCURRENT_STREAMSleaves no slot. Its writes wait for'ready'.AsyncLocalStoragereads. A tick queued under a frame runs under it.Bun.ModuleGraph.dispose()drops a graph's immediates.'ready'tick: the start channel, header coercion and failure events stay wrong.Downsides
write()whose DATA frame queues in the session still runs its callback with no store.write()from another store during a slot wait completes in therequest()caller's store (node: thewrite()caller's).runInFramecall, a write over a JS transport two.request()is byte-identical. The bundledhttp2.jsgrows 604 bytes.Notes
Found by a comparison with Node v26.3.0. No issue reports it. It completes #31825 (a stream's events run in its
request()context) for the queue that #31584 and #32488 added.Repro. One session, three callers, the peer allows one concurrent stream:
1.3.13 had no queue: it sent past the peer's limit.
git tag --containsputs #31584 in 1.4.0. Before the connect, thewrite()andend()callbacks and'finish'were wrong on 1.3.13 too (the store of theconnect()caller).Who is affected. Code that reads the store in a
write()orend()callback, in'ready'or'finish', or in a subscriber ofhttp2.client.stream.start,bodyChunkSentorbodySent, on a request that waited.'response','data','end'and'close'were right already, and so is a request that did not wait.AsyncLocalStorage.bind()on the callback is a workaround.What ran in the wrong store on main, for a request that waited:
write()callback, theend()callback,'ready','finish'and the three channels above.'error'and'close'of a request that fails at submission (ERR_HTTP2_OUT_OF_STREAMS, a header value that the encoder rejects).toString()of a header value that is not a string. Bun converts a queued request's header values at submission. The peer receivedx-user: aliceon bob's request.The store was that of the request whose stream closed and freed the slot, or the store of the
connect()caller for a request made before the connect and after a SETTINGS frame that raised the limit.The three flush callers are the
'close'listener of each submitted request,#onConnect, and theremoteSettingshandler. They are not changed. They all reach the one call that the PR frames.Node.
request()makesnew AsyncResource('PendingRequest')for each request (core.js#L1885) and submits throughreqAsync.bind(requestOnConnect.bind(...)), deferred to'connect'or at once (#L1912-L1926).requestOnConnectsubmits the request, or destroys it withERR_HTTP2_GOAWAY_SESSIONwhen the session was closed in the meantime (#L780-L786). At the stream limit node has no JS queue: it submits insiderequest()and nghttp2 holds the HEADERS frame. Bun queues in JS there, so the frame is needed for that wait too. The stream keeps the frame already, so the PR adds noAsyncResource, no field and no second holder of the store. Theclose()hunk is new parity, not part of the 1.4.0 regression.Tests. The file uses
node:testandnode:assert. Its first 11 tests run unchanged under node, and a last test runs the file under node when the machine has node 26. 11 tests are forBun.ModuleGraphand run under Bun only.http2.jsbun bd test(debug, ASAN), 7 runsnode --testThe 8 that pass on main pin what must not get worse:
destroy()before the connect cancels a queued request in the store of thedestroy()caller (as node does), the callbacks of a write over aDuplextransport run in the writer's store, 5Bun.ModuleGraphcells that complete on main, and the node run. In the test for a header value that cannot be sent, the error code is not compared: node reportsERR_HTTP2_STREAM_ERRORfrom nghttp2, Bun reportsERR_HTTP2_INVALID_HEADER_VALUE. The stores are the same.Each clause has a test that fails without it (checked on the bundled module): without the session-armed RST_STREAM 4 tests fail, without the session-armed write completion 3, without the writer's frame for that completion 1, with the frame left installed after a submission 6, without the frame on the
close()rejection 1.Bun.ModuleGraph. On main a waited request runs as the graph that drained the queue. Its callbacks see that graph asBun.ModuleGraph.current, and its unhandled failure goes to that graph'sonError. With the PR it runs as the graph that made it, also afterdispose()stopped that graph. A request that did not wait does the same, and so does a queuednode:httprequest on anAgent. Three tests pin that.The swap alone made
dispose()worse than main, because the session armed two immediates in the frame of the request, anddispose()drops the immediates of a graph:sendRstOnReady, the RST_STREAM of aclose()on a request that has no id yet. Without it the stream keeps its slot when the peer never finishes it.kDeferWriteCallback). Without it the next write of the upload never starts. A native socket completes a write on a tick, whichdispose()does not drop.The session arms both now. The write callback still runs in the frame of the writer.
A host session, the peer allows one stream, tenant B's request waits, B calls
close()on it, the peer never answers it. Does a later request of the host get the slot?write(chunk, () => end(chunk))over aDuplextransportsql.begin()keeps main's behaviour for every caller inside a graph (inCallerContext,src/js/bun/sql.ts). That early-out was measured here: it leaves "bob ran as alice" and the other graph'sonErrorfor every caller inside a live graph. The first pushed shape kept a disposed graph's request in the drainer's frame. That guard is removed: with both immediates armed by the session no cell needs it, and the leftovers of a disposed graph no longer run as the host.Measurements (debug ASAN build of this PR, and the same binary with main's
http2.js):request(), the path of a request that does not wait: bytecode identical, 945 instructions (JSC bytecode dump,cmp)._writeis identical too, 151 instructions.#flushPendingRequests: 177 -> 102 bytecode instructions, plus 86 in the new#submitPendingRequest.AsyncContextFrame.run()calls per request over a native socket: 4.00 -> 4.00 for a request that does not wait, 4.00 -> 5.00 for one that waited. Frame swaps with noAsyncLocalStoragein use: 0.00.Duplextransport a completed write pays 2run()calls, one to arm the immediate and one to run the callback. Measured with 200 requests that waited: 3.00 -> 6.00 calls per request with a body, 4.00 -> 6.00 without.close()before the connect: +50run()calls for 50 rejected requests.heapStats().objectCount, 10,000 requests).http2.js: 168,928 -> 169,532 bytes (+604). The binary embeds that text. The first pushed shape was measured withsizeon LTO release builds: bundle +619 bytes, text +619 bytes, data +0.Not changed here.
write()from another store during a slot wait completes in therequest()caller's store (node: thewrite()caller's, main: the drainer's).request()on a connected session leaves the stream corked for one tick (followed up separately). Header values are converted when the request leaves the queue, node converts them insiderequest()(node:http2: coerce outbound header values before taking a stream id #37568 is open for that).write()callback with no store, for a request that waited or not, on the client and on the server. It queues when the connection send window (65,535 bytes for all streams until the peer's WINDOW_UPDATE) or the stream's window is used up, or under backpressure. The callback is stored in the native session and called from the drain path. That is followed up with node:http2: fail a dropped write with ECANCELED, keep no write callback in native #44881, which rewrites that site. For a waited request,_writeruns in the frame that this PR installs, so a fix that keeps the frame of_writeneeds this PR first.session.settings(settings, callback)runs the callback in the session's store (followed up separately).streamRejectedByGoawaySessionwould not match node either. That path needs its own change.destroy()cancels queued requests in the store of its caller, as node does.Duplextransport in the frame that is current. For a waited request that was the drainer's store on main and is the request's own store now. Node writes as the session.close()on a stream that has an id arms its RST_STREAM immediate in the frame of the caller. A graph that closes such a stream and is disposed in the same turn keeps the slot (followed up separately).Related open PRs on the same method. None of them frames the submission. After a merge, the lines they add to the per-request block go into
#submitPendingRequest:#trackActiveRequestcall (node:http2: free a request's slot when its stream closes on the wire, abort request({ signal }) from JS only #42763) and the connection count (node:http2: don't track client streams opened by inbound HEADERS #42369).#rejectPendingRequests(). The frame belongs to itsclose()-while-connecting caller only, not to the GOAWAY caller. Pinned by "... when close() rejects it before the connect".request(), as node does. After it, the store of that conversion no longer depends on this PR.Self-review. 31 concerns before the first push. 24 are addressed in the diff, the tests or these notes:
runInFramein place of a hand-written frame swap, one-line comments, tests for the restore of the drainer's frame, the peer reset, theBun.ModuleGraphrows, the node run, and the statements above. 7 are answered in another way:sql.begin()early-out" and "add no guard to the flush" contradict each other. The measured rows above decide it: no guard, and the two immediates that free a slot are armed by the session.todorows for what stays wrong": not added. The rows are listed above.Review after the first push: a graph disposed in the middle of a waited upload over a JS transport kept its stream slot, which main did not do. The session arms the write completion now. The guard for a disposed graph is removed, because it ran the leftovers of that graph as the host.
Suites. Debug ASAN build of this PR: the new file (7 runs), the 277 vendored
test-http2-*,test-diagnostics-channel-http2-*andtest-async-local-storage-http*files, and the 12 files intest/js/node/http2/. On this machine's debug buildh2-conformance.test.ts(stream-reset floods, one GC straggler count) andnode-http2.test.js(the 9 subprocess tests of "DATA payload survives") have tests that exceed the 5 s default or the straggler bound. They do so at the same rate with main'shttp2.js(6 and 2 interleaved runs), and the 9 pass on both with a 60 s timeout. Release build (LTO) of the first pushed shape: the 12 files intest/js/node/http2/(node-http2.test.js405 pass), the 277 vendored files,AsyncLocalStorage.test.ts63,AsyncLocalStorage-tracking.test.ts75,serve-http2.test.ts93,fetch-http2-client.test.ts83,module-graph.test.ts292,module-graph-io.test.ts28,module-graph-matrix.test.ts893, grpc-js (27 of 29 files pass,test-resolverandtest-tonicfail the same way on main: public DNS names and the tonic server).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-pending-request-async-context.test.ts