Repository navigation
Conversation
|
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 (4)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughThe changes update deferred-task and microtask checkpoint handling, revise HTTP/2 flush and transport error behavior, and add regression tests for synchronous transports and injected ChangesHTTP/2 event loop and transport corrections
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The HTTP/2 scheduling and transport changes have no identified merge-blocking risk in the reviewed scope. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status Reproduced with a single file and no network: three sequential requests on one
The same check is the first new test in PR: #42353 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Updated 1:59 PM PT - Sep 11th, 2026
✅ @robobun, your commit f4442b12abc7b7cb933f14048905a6387f02cc75 passed in 🧪 To try this PR locally: bunx bun-pr 42353That installs a local version of the PR into your bun-42353 --bun |
Http2Stream._final called native.flush() synchronously. session.request() calls req.end() for an endStream request before it returns, so the HEADERS frame reached the transport while request() was still on the stack. Over a transport that delivers synchronously (an in-memory Duplex pair, a mock socket) the peer's whole response arrived before the caller could attach a 'response' listener, so the status was lost while the body and 'close' still came through. Node schedules every session write, so nothing observable happens inside request(). Both flush calls exist to stop a frame from being stranded in the cork when _final runs on the last live turn of the program. That strand has its own cause: a deferred task writes its buffer to its transport, and a JS-backed transport runs user code for that write. That code queues microtasks and nextTicks, and it can post deferred tasks of its own (the peer end of an in-process pair answering). Nothing drained or ran those afterwards, and the event loop could find no work left and exit. The checkpoint now drains again after the deferred queue when either queue holds work, and a yield task keeps the loop alive for one more checkpoint when a deferred task was posted too late for the pass.
…he transport recovered from
6031dd6 to
5034116
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
A server handler that calls end() and then pushStream() in the same tick wrote the PUSH_PROMISE after the parent's DATA(END_STREAM). RFC 9113 section 6.6 only allows a PUSH_PROMISE on a stream that is open or half-closed (remote), so a node client cancels that push. send_data records where a stream's END_STREAM DATA frame lands in the cork. The last frame of a multi-frame body takes the corked path too, so a body of any size keeps its END_STREAM corked until the tick ends. push_promise cuts the cork at that mark, writes the PUSH_PROMISE, and puts the tail back. The cut only moves DATA frames, so HPACK header blocks keep their encode order. It bumps the cork generation and marks the moved END_STREAM frames again. push_promise no longer flushes. Stacked on #42353, which removes the synchronous flush in Http2Stream._final and gives the deferred queue its liveness guarantee.
|
A review on #43578 (which carries this PR's three commits as cherry-picks) raised a point about |
This branch has not been deployed
2 participants
Problem
session.request()on a connectednode:http2client writes HEADERS while it is on the stack. Over a transport that delivers synchronously (an in-memoryDuplexfromcreateConnection), the response comes back insiderequest(), so'response'fires before a listener exists.Http2Stream._finalcallsnative.flush(), andrequest()runsreq.end()before it returns. Node schedules every session write (MaybeScheduleWrite).Fix
native.flush()calls. Frames leave at the microtask checkpoint through the session's auto-flush entry, where node writes too.drain_microtasks_with_global: after the deferred queue runs, drain pending microtasks again. If a deferred task was posted too late for the pass, a yield task gives the loop one more checkpoint before it can exit.H2FrameParser::on_auto_flush: a tick that finds a write-fatal latch the transport recovered from clears it and flushes. It used to return, and the frames corked for that tick stayed corked.node-http2.test.js(four new, main fails the first),node-http2-syscall-fault.test.ts(two new, main fails one), all 278 nodetest-http2-*tests.Background
DeferredTaskQueue, which runs once per microtask checkpoint, after the drain.is_event_loop_alive()does not count pending microtasks or deferred entries, and an in-process pair owns no handle.Notes
The latch tick, and the darwin lanes. The first CI run of this PR failed
h2-conformance.test.tsandh2-push-refusal-staged.test.tson both darwin lanes, every attempt, with the tapepreface -> recv:SETTINGS#0and no client HEADERS. At that base the session still wrote its preface to a connecting socket. macOS fails thatsend()with ENOTCONN (Linux says EAGAIN),us_socket_write_check_errorclassifies ENOTCONN as peer-gone, and the parser latchedtransport_write_fatal. The next auto-flush tick, the one the request's HEADERS were corked for, cleared the latch and returned without flushing. The_finalflush hid this. #40048 (now in the base) stopped the connect-time write, so darwin no longer latches there, but the tick still must not be spent. Reproduced on Linux withsocketFaultInjection(one ENOTCONN on the first send after connect): main strands the HEADERS of a request that stays open, this PR delivers them.Repro. Three sequential requests on one session over an in-memory
Duplexpair that delivers synchronously, against a hand-written peer that answers every HEADERS frame with:status 200andok. bun 1.4.3: the peer sees the HEADERS for streams 3 and 5 whilesession.request()is on the stack, and those two requests get'close'with the body but no'response'. node v26.3.0 and this PR:on the stack: falseand'response' 200for all three. Over a kernel socket nothing can be delivered insiderequest(), sohttp2.connect(url)never showed the symptom.Why the event-loop change is in this PR. Removing only the two
flush()calls regresses cases that pass today (the "flushes removed only" column below): the frames are written, but the process exits before the client stream reports them. The flushes were a workaround for that strand, and the strand is reachable on main without them (the 112 failing cases in the second matrix).Why the fix is in the checkpoint and not in the http2 parser. The
DeferredTaskQueuealready documents that a callback can reach user JS. Every other user of the queue avoids resolving promises inside its callback by posting a task (FlushPendingFileSinkTask,StreamPending), which is the same constraint worked around per call site. With the drain and the yield task in the checkpoint, the queue's contract holds for any callback.Self-review. Checked:
take_unrun()can report a late entry that a pass did run (a swap-remove pulled it forward). The cost is one empty loop iteration. It cannot miss one, because onlypost_taskof a new key sets the mark andrun()clears it before the first callback. An entry that returnstrueand stays registered never sets the mark, so a sink waiting on a writable poll does not spin the loop. On a loop that is closed for tasks,enqueue_task_after_yieldreleases the task at once. Native-socket sessions post from inside a pass only when a flush dispatches a write callback that writes again. Those frames used to wait for the next unrelated checkpoint and now get one on the next iteration.Measured against node v26.3.0 over
duplexPair. Two matrices, each run as a separate process per case, checking that'response','end'(with the full body),'close', and the server's'close'all fire before exit.'connect',setImmediate,setTimeout, or after a previous stream closed, times 7 server response shapesnextTickand microtask, times a server that answers from 1 to 5 nestednextTick/ microtask / promise hops, times 4 response shapesThe 112 cases stock bun fails in the second matrix are all "trailers sent from a deferred callback": the pre-existing strand, which the synchronous flush never covered.
Why a yield task and not a second pass in the same checkpoint. Looping the deferred queue until it is quiet would run a whole in-process conversation inside one checkpoint, ahead of timers and I/O, and two chatty in-process peers could starve the loop. Node submits each side's frames from its own immediate, so I/O interleaves. One checkpoint per loop iteration gives the same fairness.
Cost. The second drain is guarded by
JSC__JSGlobalObject__hasPendingMicrotasks(two loads), and the yield task is only queued when a deferred callback posted a new entry during the pass, which native-socket flushes do not do. A deferred-task-heavy loop (200kFileSinkwrites, one persetImmediate, debug+ASAN, interleaved runs): 27.3 / 26.9 / 27.5 / 26.9 s on main, 26.8 / 27.4 / 27.7 / 27.1 s with the change. An unguarded second drain cost about 9% on the same loop. 1500 sequential h2 requests over TCP, same build with and without the event-loop change: 5.45 / 5.60 / 5.62 s and 5.55 / 5.54 / 5.65 s.Reset sockets. A peer that resets the connection after answering is reported the same way before and after (
session error: ECONNRESET): the same write path runs, from the scheduled flush instead of fromrequest().Other suites, each compared with a build of main:
test/js/web/fetch,test/js/web/streams,test/js/node/stream,test/js/bun/http,test/js/bun/io,test/js/third_party/grpc-js,test/js/third_party/http2-wrapper, and 1347 node tests matchingtest-{stream,net,timers,process-next,http-,https-,tls-,worker,child-process}*. Failures are identical with and without the change (IPv6, proxy, and CA-store tests that need network or host setup, ASAN leak thresholds, and files that exceed a 60 s cap under debug+ASAN and pass serially).no test proof · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/http2/node-http2-syscall-fault.test.ts