Repository navigation
Conversation
|
Status
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughHTTP response backpressure now uses response-local drain state. Writable properties account for response lifecycle, socket attachment, and buffered output. Pipeline replay preserves owed drain state. Tests cover writes, streams, pipelining, and socket behavior. ChangesHTTP response backpressure
Suggested reviewers: Priority: ⬆️ High Merge Risk: ⚪ Minimal · up to
|
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/_http_server.ts— pre-existing: a pipelined response that buffered writes while queued sends them out of order, or drops them, when a 'socket' listener writes or ends it. advanceResponsePipeline clears kPipelinedQueuedState at src/js/node/_http_server.ts:2893 and emits 'socket' before the ops replay at line 2943. A res.write() from that listener takes the direct handle path at line 3646-3722, so its bytes precede the buffered chunks; a res.end() there makes every replayed write a write-after-end and the chunks are lost. Fix: flush the not-yet-replayed ops before any direct write()/end()/_writeRaw on the response, so order holds from the 'socket' event on. The new test at test/js/node/http/node-http-backpressure.test.ts:1572 asserts only the body length, so it passes with the wrong order. [also at: src/js/node/_http_server.ts:2922 - pre-existing: a pipelined client gets the body bytes of a queued response out of order when the response's 'socket' listener calls res.write(). advanceResponsePipeline clears kPipelinedQueuedState at src/js/node/_http_server.ts:2893 and emits 'socket' (assignSocketInternal / res.assignSocket at…]Why this was flagged
Trigger: a second request pipelined behind an unfinished response; its handler calls res.write(chunk) several times while queued (bufferPipelinedWrite, src/js/node/_http_server.ts:3031) and has res.on('socket', () => res.write('x')) or ends the response in that listener. When the first response finishes, advanceResponsePipeline runs queue.shift() and res[kPipelinedQueuedState] = undefined (lines 2892-2893), then assignSocketInternal emits 'socket' (line 164) before the replay loop at line 2943. Inside the listener, ServerResponse.prototype.write finds kPipelinedQueuedState undefined at line 3646 and writes through handle.cork/handle.write at line 3703-3722, so the wire carries head, 'x', then the buffered chunks, then the tail. With res.end('tail') in the listener instead, finished is set at line 3563 and each replayed write hits hasServerResponseFinished at line 3638 (write-after-end), so the buffered chunks never reach the client. The base branch has the same dequeue-assign-replay order, so this is pre-existing; the added test at line 1572 compares only body.length, not the byte order, so it does not catch it.
Verification: advanceResponsePipeline runs queue.shift(); res[kPipelinedQueuedState] = undefined; (src/js/node/_http_server.ts:2892-2893), then assignSocketInternal (2913) emits 'socket', and only afterwards replays queued.ops (2943-2956). A res.write("x") from the 'socket' listener sees this[kPipelinedQueuedState] === undefined (3646) and goes straight to handle.write (3702-3726), so its bytes reach the wire before the replayed buffered chunks.
|
On the finding about a The test at |
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.
Beyond the inline findings, I also checked whether the new onwritable !== undefined gate in emitDrainIfOwed could swallow the 'drain' that the native drain callback itself emits via allowWritesToContinue — it cannot: on_drain_corked in src/runtime/server/NodeHTTPResponse.rs clears the cached onwritable slot before invoking the callback, so get_on_writable reads undefined at that point and the emit goes through.
Extended reasoning...
The change reworks ServerResponse 'drain'/need-drain bookkeeping in src/js/node/_http_server.ts and adds ~1,200 lines of tests; it touches no security-sensitive surface. Three confirmed findings are posted inline, and this note only records one additional concern that was examined and ruled out from the native handle's onwritable implementation.
…response counts only its own bytes writableNeedDrain was derived from the buffer of the connection, the armed drain callback and the per-turn count. A response that waits behind another one read the bytes of the response ahead, so pipe() waited for a 'drain' that never came. write() now stores kNeedDrain where it returns false. One emitter tests the flag, waits while the drain callback of the handle is armed, clears the flag and emits 'drain'. The getter reads the flag for a response with a handle. writableLength counts the unsent bytes of the connection only for the response that has the socket. _send() no longer replaces the armed drain callback.
… replayed write At the turn of a queued response, a kept write that the cork buffer took whole reports no backpressure. When the socket then takes only part of it, the owed 'drain' now waits for the drain callback of the handle. The destroyed-socket tests pin what write() returns by the Node.js version.
The native write takes nothing for a closed socket and returns 0, so the callback ran as if the bytes were sent. Node's _writeRaw() drops it. The destroyed-socket tests state the value of Bun, and apply the Node.js version switch only under Node.js.
b33689e to
fdbc86d
Compare
|
Updated 9:23 AM PT - Oct 16th, 2026
✅ @robobun, your commit fdbc86dc9179b3bf43c6ba6dcb31565125540a1f passed in 🧪 To try this PR locally: bunx bun-pr 44877That installs a local version of the PR into your bun-44877 --bun |
There was a problem hiding this comment.
I re-reviewed the three new commits and found no bugs; the earlier inline points (the _send callback on a closed handle, the process.versions.node gate in the test, and arming onwritable after a replay that left bytes in the socket) now appear addressed in the code. Because this reworks drain/backpressure ordering across the pipelined, foreign-duplex and bodyless paths of node:http and documents a deliberate divergence from Node in writableNeedDrain, a maintainer look would still be worthwhile.
What was reviewed:
- Every
return falseinServerResponse.prototype.writeandbufferPipelinedWritenow setskNeedDrain; the lone exception (write-after-end viahasServerResponseFinished) matches Node, which also leaves the flag untouched there. - The
finallyrestore ofkNeedDraininadvanceResponsePipeline: replayed ops go through the handle directly, so the overwrite cannot discard a flag set during the replay. _sendnow routes the user callback throughkPendingCallbackson backpressure and drops it when the handle is closed; the nextTick callback on a synchronous handler whose bytes are still in the cork buffer was considered and ruled out as a regression (base never called it at all).- Removing
_flush()fromassignSocketwhen a native handle exists:outputDatais empty on that path, and the queued 'drain'/'prefinish' are handled after the replay.
Extended reasoning...
The diff touches src/js/node/_http_server.ts (about 90 lines) plus roughly 1,250 lines of new tests in node-http-backpressure, node-http-syscall-fault and the 19111 regression test. It changes how ServerResponse tracks and emits 'drain' (a per-response kNeedDrain flag, a single emitDrainIfOwed emitter, writableLength scoped to the socket owner, _send no longer clobbering the armed drain callback), with no security-sensitive surface. The bug hunt ran dry with no findings and the inline comments from the two prior runs were addressed by the new commits, but the change is a non-trivial rework of a hot-path node:http module with a documented intentional divergence from Node for serverless responses, which a maintainer should sign off on rather than an automated approval.
|
Closing: superseded by #46014. That PR has this branch merged in, with its tests, and adds the byte gate for |
Problem
node:httpserver never answers a pipelined request that streams withpipe()while the response ahead has unsent bytes. Node.js v26.3.0 answers.ServerResponse#writableNeedDrain(src/js/node/_http_server.ts:3832) read the buffer of the connection. A waiting response saw the bytes ahead of it, andpipe()waited for a'drain'that never came.Fix
write()storeskNeedDrainwhere it returns false. The getter reads it.'drain'. It tests the flag, returns while the drain callback of the handle is armed, then clears the flag and emits.writableLengthcounts the bytes of the connection only for the response that has the socket.node-http-backpressure.test.ts(79 new tests, 48 fail on main), all oftest/js/node/http, 533 vendored scripts. Self-reviewed: 19 concerns raised, 18 addressed (Notes).Background
pipe()readswritableNeedDrainbefore its first write. If true, it waits for'drain'.write()that returns false sets it, its'drain'clears it.bufferedAmountterm, or answer 0 natively for a queued response. Both keep a derived getter, which reads true after a replay.Downsides
ServerResponsewithout a server still reads false after a refusedwrite(). Node.js reads true. Main's light-my-request flows keep their answers.'drain'(main: two).'drain'costs one more native getter call.node/_http_server.jsgrows by 700 bytes.Notes
Reach. No issue reports this. It needs a client that pipelines (undici with
pipelining: 2, a proxy), or megabytes in front of the body that nowrite()put there (a large head, a rawres.socket.write()), or a server withhighWaterMark: 0. The pipelined hang is on 1.3.14, 1.4.0, 1.4.2 and main.Repro (exit 1 on main, exit 0 on Node.js v26.3.0 and on this branch. The same over TLS: 1, 0, 0):
The fresh response to
/b, before it writes anything:writableNeedDrainwritableLength/a)The same getter, other entry points. Each one hangs or loses bytes on main, and has a test:
res.flushHeaders()or a largeres.socket.write(), thenpipe(){ highWaterMark: 0 }: every fresh response read trueServerResponsewithout a server on a socket withhighWaterMark: 0: it read true before any write, andpipe()into it never started (test/regression/issue/19111.test.ts)writableNeedDrainat the'socket'event of a response whose queued write returned false (main read false there)Writable.toWeb(res)after a replay that met native backpressure: main read true with nowrite()that returned false, and the adapter did not write the chunkThe emit rule.
'drain'on a message when the flag is set and the message has nothing left to send (socketOnDrain).'drain'owed: the drain callback of the handle, the end-of-turn count, and the turn of a queued response. All three callemitDrainIfOwed.while (res.writableLength > 0) await once(res, "drain")gets its only'drain'too early and hangs.'drain'comes at once when nothing is left to send, like in Node's_flush(). One tick later was too late: a write throughWritable.toWeb(res)in that tick saw the flag and lost its chunk.'drain'over those bytes, with nothing left to emit a second one. The turn now arms the drain callback of the handle when the handle still holds bytes.res._send()gave its callback to the handle as the drain callback. That replaced the armed callback of an earlierwrite(), and the'drain'of that write never came. It now passes the same drain callback aswrite(), and its own callback waits for the socket. A raw write to a socket that is gone drops its callback, as in Node.js.Why
_flush()is gated.OutgoingMessage#_flushemits'drain'whenkNeedDrainis set andwritableLengthis 0.assignSocket()ran it before the replay of the kept writes. With the stored flag, a'drain'listener that ends the response would run ahead of those writes. A response with a handle keeps nothing inoutputData, so_flush()has nothing else to do there. The same gate gives a subclass that overridesassignSocket()one'prefinish'for a response that ended while queued (main: two, Node.js: one).A response without a server. This is a deliberate difference from Node.js, for a maintainer to confirm.
fastify.inject()build aServerResponsethemselves and give it a socket that no server watches. Nothing brings the'drain'of that socket to the response.write(). A laterpipe()into the response then never starts.fastify.inject()flows that answer on main stop answering, as on Node.js. With the gate on the handle, all 37 flows give the result of main.Existing test rewritten.
but not on a HEAD responseandbut not on a 204 responsecomputed their precondition asres.writableLength + req.socket.writableLength > 0, with the comment "Bun counts the unsent bytes of the earlier response on this response". That expression pinned the defect. The precondition now reads thewritableLengthof the earlier response, which is above 0 on main, on this branch and on Node.js.Measurements. Release-mode builds of main and of this branch, Linux x64. The bytecode table and the bundle size are from the current merge base (
5866b5f13d). The other rows are from release binaries of main at5749c3129and of this branch before the rebase.Native getter calls on the response handle (counting accessors):
res.writableNeedDrainpipe()setupwrite(chunk)write(chunk, cb)end(chunk)writableLengthon a queued response'drain'Bytecode instructions for each edited function (
BUN_JSC_dumpGeneratedBytecodes):ServerResponse.prototype.writeflushWriteAccountingNTallowWritesToContinueemitPipelinedDrainNTemitDrainIfOwedbufferPipelinedWriteadvanceResponsePipelinequeuePipelinedResponsewritableLengthgetterwritableNeedDraingetterServerResponse.prototype._sendServerResponse.prototype.assignSocketServerResponse.prototype.flushHeadersInstructions that run, read from the two listings:
write()that returns true: 1 fewerwrite()that returns false at the mark: 3 moreend("hello"): +0 per request. No edited function runs.end(): +10 per request, all in the call at the end-of-turn count. The four writes net 0.end("hello"): +0 per request. No edited function runs.perf_event_openreturns ENOENT), andperfandvalgrindare not installed.Events and values:
'drain'events for one backlog'drain'events after a replay whose writes all returned true'prefinish'events for a queued, ended response of a subclass that overridesassignSocketwritableNeedDrainat the'socket'event after a queuedwrite()returned falseServerResponsebefore writes, after four 16 KB writes, at finishwritableNeedDrainon a response without a serveroutgoingDataadded byflushHeaders()on a queued response, bytesnode/_http_server.js, bytesOf the 23 answered scenarios, 18 are equal to Node.js in every recorded fact. Main answers no scenario that this branch does not. The 4 that stay unanswered are in the kept calls of a queued response (see below).
test-http-pipeline-flood.jsandtest-http-pipeline-socket-parser-typeerror.jsexit 0.Tests.
node-http-backpressure.test.ts: 79 new tests. 48 fail on main. All 79 pass on Node.js v26.3.0 (cd test && node node_modules/vitest/vitest.mjs run --globals js/node/http/node-http-backpressure.test.ts -t "the response's own|waits for the socket|every byte of a response arrives|but not on a").node-http-syscall-fault.test.ts: 3 new tests under short sends (ASAN builds). Two fail on main. The third one is the only test that reaches the store inwrite(chunk, callback)for a write that the cork buffer took whole and the socket took in part.test/regression/issue/19111.test.ts: 2 new tests. One fails on main. The other one pins the difference from Node.js for a response without a server.!queued.endedand!hitBackpressureat the turn of a queued response. Main has both, and each only saves a tick.req.socket.destroy(),write()returns false on Node.js 26.3 and true from 26.9 on. The test accepts both.node:http. Up to seven older tests in the twonode-httpfiles spawn one and need--timeoutabove the default 5 s on a debug build, on main too.Suites. Release builds of this branch and of main, before the rebase onto
5866b5f13d: the 38 files oftest/js/node/http(onlynode-http-backpressure.test.tsdiffers), 57 more files (test/js/node/http2,test/js/node/stream,test/js/web/streams, express, fastify, body-parser, hono, socket.io, msw, st, four regression tests, of which only19111.test.tsdiffers), and 533 vendoredtest-http-*,test-https-*,test-stream-pipe*,test-stream-pipeline*,test-http2-compat*scripts (all exit 0 on both). Debug build at the current head: the three changed test files, 175 pass with--timeout 30000.Self-review. 19 concerns, 18 addressed. Not taken: replace five Bun-only matcher calls in older tests of the file, so that the whole file runs on Node.js. Those tests are outside this change, and the command above runs the new ones.
Same class, left out.
'drain'(src/js/node/_http_server.ts:1732), two for one backlog. node:http: order req.socket.write() behind the corked response head #35018 removes it.writableCorked,cork()anduncork()act on the connection: node:http: don't leave the server socket Duplex corked across kept-alive requests #35664, node:http: deliver every chunk that a corked OutgoingMessage accepts #44676.ServerResponse.prototypekeeps 12 own accessors where Node.js has none.writableHighWaterMarkis in node:http: run a response's 'finish' connection step on the socket the dispatcher detached it from #44457.write()in its'socket'listener goes out ahead of the kept writes, andres.write(123)returns true and throws at the replay. The branchrobobun/7f2a07bf/http-queued-response-sync-throwmoves the kept calls into the handle.Writable.toWeb()does not write a chunk whilewritableNeedDrainis true (src/js/internal/webstreams_adapters.ts:287, the same line as in Node.js). The stored flag removes one way into it.writableLengthat the'socket'event is 0 until the replay. A queued response counts an estimate of its head and no chunk framing.writableLengthdoes not count what the cork buffer holds. At the turn of a queued response, the socket can take the first flush whole and only part of a later kept write that an outer cork still holds. The'drain'has come by then, on main too. It needs the cork buffer in the count, or the nativesendCorked()that node:http: make res._send() the stored head plus _writeRaw(), as in Node.js #44871 and node:http: send a 1xx response when it is written #44468 add.flushHeaders()on a queued response sends no head at the turn of the response until the first body write. node:http: make res._send() the stored head plus _writeRaw(), as in Node.js #44871 sends it.writableLengthof a response without a server reads 0 while its socket still holds bytes. WithContent-Length,write(body)andend(), its'finish'comes before the socket has taken the body. Node.js waits. Main has the same. This PR leaves a response without a server as it is on main.req.socket.writableLengthreads 0 on the native path while megabytes are unsent.highWaterMarkabove what the socket takes: a write after such a replay returns false where Node.js returns true.emitDrainIfOwedis not shared with_http_client.tsandOutgoingMessage#_flush. They carry Node's text, one guard each, as in Node.js.Other open PRs on these lines.
res._send()and the queued branch offlushHeaders(). Its raw write keeps an armed drain callback, and its_send("")counts the head. Whichever lands second drops its copy of those two hunks. With this PR, a raw write that returned false gets no'drain'and leaveswritableNeedDrainfalse, as in Node.js.res[fakeSocketSymbol]?.writableEndedtest toemitDrainIfOwedas a return before the clear.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/http/node-http-syscall-fault.test.ts, test/js/node/http/node-http-backpressure.test.ts