node:http: queue pipelined responses on fallback connections instead of throwing ERR_HTTP_SOCKET_ASSIGNED - #36991
node:http: queue pipelined responses on fallback connections instead of throwing ERR_HTTP_SOCKET_ASSIGNED#36991robobun wants to merge 8 commits into
Conversation
…of throwing ERR_HTTP_SOCKET_ASSIGNED
WalkthroughHTTP/1 response pipeline handling now shares queueing, advancement, abort, and backpressure helpers across native and fallback sockets. The fallback server queues responses during socket reuse and advances them after completion. Tests cover ordering, backpressure, connection cleanup, upgrades, and proxy address binding. ChangesHTTP/1 response pipeline
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…ion dies Share the pipelining helpers with internal/http1_server_fallback through internal/http instead of the node:_http_server module exports, and mirror the native socket close path for responses still queued on a foreign duplex: destroy them and their requests so both emit 'close'.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks — ff2ea1b addresses the two concerns from my earlier pass: abortQueuedPipelinedResponses now runs from the fallback socket 'close' listener (with a hang-guard test), and the pipeline helpers moved to internal/http so node:_http_server's exports are unchanged. I didn't find anything new this round, but this reworks advanceResponsePipeline's dispatch for both socket kinds and there's an acknowledged follow-up on the flood guard plus the comment-cop nits, so worth a human look before landing.
What was reviewed:
queuePipelinedResponse/abortQueuedPipelinedResponsesextractions are byte-identical moves; native#onCloseand dispatcher paths behave the same.advanceResponsePipeline's newinstanceof NodeHTTPServerSocketbranch keeps the native code path unchanged;releasePipelineOutgoingDataon a foreign duplex is a no-op via optional chaining, not a throw.- Fallback
'finish'→detachSocket→advanceResponsePipelinechain and thefinished=false-before-assignSocketordering for already-ended queued responses.
Extended reasoning...
Overview
This PR fixes an uncaught ERR_HTTP_SOCKET_ASSIGNED on the JS HTTP/1 fallback server path (server.emit('connection', duplex) and http2's allowHTTP1 ALPN fallback) when pipelined requests arrive. It does so by having the fallback path share the native path's per-connection response queue: three helpers (queuePipelinedResponse, advanceResponsePipeline, abortQueuedPipelinedResponses) are extracted in _http_server.ts and registered onto an internal object in internal/http; the fallback's headers-complete handler queues instead of assigning when socket._httpMessage is set, its 'finish' handler advances the pipeline, and its 'close' handler aborts the queue. advanceResponsePipeline gains a non-native-socket branch that skips startPipelinedResponse and clears finished before assignSocket so _flush doesn't emit an early 'prefinish'. Four files touched, two new tests.
Since my last review
ff2ea1b addressed both actionable items from my earlier pass: the queued-response abort loop is extracted and wired into the fallback 'close' listener (covered by the new "aborts queued pipelined responses when the connection dies" test, which hangs without the fix), and the helpers are shared through http1ServerPipeline in internal/http rather than the node:_http_server default export. The flood-guard gap I flagged is acknowledged as a deliberate follow-up, with a reasonable rationale (needs pausePipelineReads/releasePipelineOutgoingData to learn a foreign-duplex branch, and pre-existing backpressure gaps in the fallback handle).
Security risks
Low-to-moderate. The fallback path is network-reachable via http2 allowHTTP1, and the deferred flood guard means a pipelining client can queue unbounded responses on that path — but that's strictly no worse than main (which crashes on the second request), and the author has scoped it as a follow-up. No auth/crypto/permissions surface.
Level of scrutiny
Medium-high. This is core Node-compat HTTP server dispatch — advanceResponsePipeline runs for every pipelined response on every native connection too, and the change restructures its control flow around an instanceof check. I verified the native branch's body is unchanged relative to the pre-PR code, and the two extractions are pure moves. The fallback branch's finished=false clearing before assignSocket is subtle enough (it interacts with _flush/prefinish and the second finished=false clear before op replay) that a maintainer familiar with the ServerResponse finish machinery should confirm the ordering.
Other factors
- The comment-cop bot flagged eight multi-line comments across the diff. Most explain Node-compat mirroring rather than justify workarounds, but they run 4-7 lines each against the repo's one-line preference; the author may want to trim them.
- Test coverage is good: three pipelining scenarios (delayed, out-of-order chunked, synchronous chain) plus the abort-on-close case, all over
duplexPair()so they exercise the fallback path specifically. - The
http1ServerPipelinelate-registration pattern (populated when_http_serverloads, read after the fallback'srequire('node:http')) works because module loading is synchronous, but is a slightly unusual sharing mechanism — worth a maintainer's nod.
There was a problem hiding this comment.
Thanks — ff2ea1b addresses both actionable findings from my earlier pass (queued responses now aborted on fallback socket close via the shared abortQueuedPipelinedResponses, and the pipeline helpers moved off the node:_http_server export surface into internal/http), and the flood-guard deferral is reasonable given the pre-existing backpressure gap on this path. No new issues found this round.
That said, this is a non-trivial refactor of advanceResponsePipeline — it now branches on socket type and manipulates res.finished ahead of assignSocket to keep _flush from firing an early 'prefinish' on the fallback path — plus a module-load-order dependency for the http1ServerPipeline late registration. Worth a human look given it's core HTTP/1 pipelining and reachable via the http2 allowHTTP1 ALPN fallback.
What was reviewed: the extracted queuePipelinedResponse/abortQueuedPipelinedResponses are byte-identical moves from their original inline sites; the ops-replay loop after the new fallback assignSocket still runs and re-sets finished via the replayed end(); the fallback close listener now mirrors the native #onClose abort loop; the new abort test would hang without the fix.
Extended reasoning...
Overview
This PR fixes an uncaught ERR_HTTP_SOCKET_ASSIGNED crash on the JS HTTP/1 fallback path (server.emit('connection', duplex) and http2's allowHTTP1 ALPN fallback) when pipelined requests arrive. It does so by having the fallback path reuse the native dispatcher's per-connection pipelined-response queue (kPipelinedQueuedState / kPipelinedResponses). Concretely: _http_server.ts extracts the inline queue setup into queuePipelinedResponse(), extracts the native socket's close-time abort loop into abortQueuedPipelinedResponses(), and teaches advanceResponsePipeline() a non-native-socket branch. http1_server_fallback.ts calls these instead of unconditionally assignSocket. The helpers are shared via a late-populated http1ServerPipeline object in internal/http rather than as node:_http_server exports.
Since my previous review, ff2ea1b addressed the 🔴 (queued responses leaked on fallback socket close — now aborted via the shared helper, with a new hang-if-broken test) and the 🟡 export-surface nit (helpers moved to internal/http). The remaining 🟡 (flood-guard not carried over) was deferred with a reasoned explanation that a correct port also needs pausePipelineReads/releasePipelineOutgoingData to learn a non-native pause/resume branch, and that the fallback handle already ignores transport backpressure for the in-flight response — both pre-existing and better done together. 9bbddc8 tightened comments.
Security risks
Low but non-zero. The http2 allowHTTP1 ALPN fallback is network-reachable, and this PR enables unbounded per-connection response queueing on that path without the kOutgoingData read-pause flood guard the native path applies. However, before this PR the same input crashed the process outright, so this is strictly an improvement; the flood-guard gap is acknowledged as a follow-up. No new parsing of untrusted input; no auth/crypto/permissions changes.
Level of scrutiny
Moderate-to-high. This is core node:http server pipelining — response ordering, socket-lifecycle transitions, and the finished/'prefinish' state machine are all subtle and historically bug-prone. The advanceResponsePipeline change introduces an instanceof NodeHTTPServerSocket branch where the fallback arm clears res.finished before assignSocket (so _flush doesn't emit an early 'prefinish'), then relies on the shared ops-replay loop to call res.end() and re-set it. I traced this and it looks correct (the replay loop's own finished = false reset is redundant but harmless on this path), but the interaction with user ServerResponse subclasses overriding assignSocket, and with the _flush → 'prefinish' → 'finish' chain, deserves human eyes.
The http1ServerPipeline late-registration pattern depends on require('node:http') having already loaded _http_server before the fallback destructures the helpers — the fallback's own require('node:http') guarantees this, and it's commented, but it's an unusual coupling.
Other factors
Test coverage is good: four scenarios including async-handler pipelining, out-of-order completion with buffered chunked writes, three synchronous requests, and the abort-on-close case I flagged. The extracted queuePipelinedResponse and abortQueuedPipelinedResponses are verbatim moves from their inline originals, so the native path's behavior is unchanged. The comment-cop bot left several generic "paragraph-long comment" flags; the flagged comments explain genuinely non-obvious behavior (Node's parserOnIncoming queue semantics, why finished must be cleared before assignSocket) and read as appropriate to me, but that's a style call for a human.
Not approving because this touches critical HTTP server internals with subtle state-machine interactions — well beyond the "simple, mechanical, or obvious" bar.
|
CI state across the seven builds on this PR (89328 through 89382, latest at d02dd2f: 195/196 jobs green): every lane that runs this diff is green, including all new pipelining tests on every platform. The one recurring red is unrelated to this change: test/js/node/async_hooks/AsyncLocalStorage-tracking.test.ts fails on the debian 13 x64-asan lane with an ASAN leak report in boringssl RSA keygen that also reproduces on main (tracked separately). Ready for review. |
… queued-destroy and allowHTTP1 pipelining behavior Mirror Node's parserOnIncoming read gate on fallback duplexes: pause reads when the transport or the bytes buffered on queued pipelined responses pass the high water mark, resume from the pipeline advance or socket drain. Gate req._read so body reads cannot defeat the pause. Also pin two behaviors with tests: a destroyed queued response resets the connection (deliberate divergence from Node v26's wedge, matching the native path), and the http2 allowHTTP1 ALPN fallback serves pipelined requests in order and aborts queued ones when the connection dies.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/node/http/node-http.test.ts`:
- Around line 4325-4327: Replace the unbounded polling loop after releaseFirst()
with a completion promise that resolves when the Nth HTTP/1.1 200 response is
observed. Wire clientSide error events and premature close events to reject the
same promise, then await it after releaseFirst(), preserving the test’s existing
response-count condition.
In `@test/js/node/http2/node-http2.test.js`:
- Around line 3930-3959: The test promise created in the HTTP/2 fallback test
currently ignores TLS socket failures. Capture its reject handler from
Promise.withResolvers and wire socket.on("error") to reject the test promise,
while retaining the existing aborted-resolution behavior and cleanup.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7611fe61-5d27-4dd7-9305-462492125db2
📒 Files selected for processing (5)
src/js/internal/http.tssrc/js/internal/http1_server_fallback.tssrc/js/node/_http_server.tstest/js/node/http/node-http.test.tstest/js/node/http2/node-http2.test.js
…on address family A bare localhost can bind ::1 while the client resolves 127.0.0.1 first; neither Node nor Bun falls back across families, so the test failed with ECONNREFUSED on dual-stack hosts where the two resolutions diverge.
Mirror the native socket close path and Node's socketOnClose abortIncoming: destroy socket._httpMessage's request (ConnResetException when it has an error listener) before aborting the queued pipelined responses, so req 'close' fires for the in-flight request too.
…se instead of advancing the pipeline Node's resOnFinish _last branch and the native onResponseFinishHandleSocket end the connection when the finished response advertised Connection: close (kMustCloseConnection); the fallback finish path only advanced, so a queued pipelined response was replayed after the final response (RFC 9112 9.6). Gate the advance on the flag and destroySoon/end the socket; the close path then aborts the queued responses like the native path.
|
Updated 6:52 PM PT - Aug 5th, 2026
❌ @robobun, your commit d02dd2f has 1 failures in
🧪 To try this PR locally: bunx bun-pr 36991That installs a local version of the PR into your bun-36991 --bun |
There was a problem hiding this comment.
No further issues on d02dd2f — the kMustCloseConnection gate in onFallbackResponseFinish and its test look right. Deferring to a human: this reworks the shared pipelining path (advanceResponsePipeline now branches on socket type) and adds queue/abort/read-gate semantics to the network-reachable allowHTTP1 fallback.
Checked this run: the Connection: close gate ends the socket before advancing (queued responses are aborted via the close listener, matching Node's resOnFinish _last branch); the native-path refactor (queuePipelinedResponse / abortQueuedPipelinedResponses extraction) is behavior-preserving vs the inline code it replaced; releasePipelineOutgoingData's new fallback socket.resume() branch is guarded so native sockets still route through response.resume(); the res.finished = false reset before fallback assignSocket is scoped to queued.ended so it can't affect an un-ended queued response.
Extended reasoning...
Overview
The PR fixes an uncaught ERR_HTTP_SOCKET_ASSIGNED crash when pipelined requests arrive on the JS HTTP/1 fallback path (server.emit('connection', duplex) and http2 allowHTTP1). It extracts the native dispatcher's per-connection response queue (queuePipelinedResponse, abortQueuedPipelinedResponses) into shared helpers registered on an internal http1ServerPipeline object, teaches advanceResponsePipeline to handle non-native sockets, and wires the fallback connection listener to queue/advance/abort responses and gate reads on outgoing backpressure. ~230 lines of source across _http_server.ts, http1_server_fallback.ts, and internal/http.ts, plus ~260 lines of new tests covering ordering, out-of-order completion, connection-death abort, Connection: close termination, destroyed-queued-slot reset, and the read gate.
Security risks
The allowHTTP1 fallback is network-reachable over TLS, so the read-gate addition (maybePauseFallbackReads) is a DoS mitigation — without it a pipelining client could buffer queued-response bytes without bound. The gate mirrors Node's parserOnIncoming check and is covered by a test that asserts the pause and subsequent drain. No new parsing of untrusted input; the change reuses the existing HTTPParser and renderNativeHeaders paths.
Level of scrutiny
High. This is production node:http server code on a hot path, and advanceResponsePipeline is now shared between native and fallback connections with a new instanceof NodeHTTPServerSocket branch — a mistake here affects every native pipelined response too. Three prior automated review rounds each found a real issue (missing queued-response abort on close, missing read-gate, missing in-flight request abort, missing kMustCloseConnection gate before advance), all now fixed with tests.
Other factors
Test coverage is thorough (six new tests plus two http2 fallback tests, wire output verified byte-identical to Node v26). The native-path changes are refactor-only (inline code → named functions with identical bodies). The one deliberate Node divergence (reset the connection when a queued response is destroyed, vs Node wedging until requestTimeout) is documented and tested. All prior review comments (mine, CodeRabbit's, comment-cop's) are resolved. The remaining unresolved comment-cop note on line 354 is the automated "paragraph-long comment" heuristic firing on an explanatory comment, not a code issue.
2 participants
Problem
On the JS HTTP/1 fallback path (
server.emit('connection', duplex)on a plainhttp.Server, and http2'sallowHTTP1ALPN fallback), a pipelined or fast keep-alive second request crashes with an uncaughtERR_HTTP_SOCKET_ASSIGNED(on the allowHTTP1 TLS path it surfaces asclientErrorand the connection is killed with no response bytes):Node v26 serves both requests in order.
Cause
connectionListenerHTTP1(internal/http1_server_fallback, added in #34432) callsres.assignSocket(socket)unconditionally for every parsed request. The previous response only releases the socket from its'finish'listener, and'finish'is deferred a tick, so whenever a second request's headers complete before that (always, for two requests in one TCP segment),assignSocketthrows. The throw escapesparser.execute()inside the socket's'data'handler, so it surfaces as an uncaughtException. A synchronousres.end()in the handler does not avoid it.The native (uWS) server already handles this with a per-connection response queue (
kPipelinedQueuedState/advanceResponsePipelinein_http_server.ts, mirroring Node'sparserOnIncomingoutgoing queue andresOnFinish); the fallback path never used it.Fix
Reuse that queue for fallback connections, matching Node's
_http_server.jsshape:_http_server.ts: extract the dispatcher's inline queue setup intoqueuePipelinedResponse()and the native socket close path's abort loop intoabortQueuedPipelinedResponses(), and teachadvanceResponsePipeline()to advance a non-native socket: no nativestartPipelinedResponseto call (each fallback response handle writes to the socket itself), assign via the prototypeassignSocketso the'close'listener a plain stream needs is installed, and clearfinishedbefore assigning so_flushdoes not emit an early'prefinish'for a response that was ended while queued. The native branch is unchanged.internal/http1_server_fallback.ts: on headers-complete, queue the response whensocket._httpMessageis still set instead of callingassignSocket; after the finish path detaches the socket, either end the connection when the response advertisedConnection: close(kMustCloseConnection, Node'sresOnFinish_lastbranch; the queued responses are then aborted by the close path, RFC 9112 9.6) or advance the pipeline so the next queued response is assigned and its buffered writes are replayed; on socket close, abort the in-flight request and the responses (and their requests) still queued, like the native path and Node'ssocketOnClose.parserOnIncomingread gate to the fallback: pause reads when the transport or the bytes buffered on queued responses pass the socket's high water mark, resume from the pipeline advance or socket'drain'(Node'ssocketOnDrain). Without it a pipelining client could buffer responses without bound; with it dispatch stops at the high water mark like Node.The helpers are shared with the fallback module through
internal/http(registered when_http_serverinitializes), sonode:_http_server's exports are unchanged.Buffering while queued (write/end/1xx, backpressure accounting,
res.socket === null) comes from the existingkPipelinedQueuedStatemachinery, so both server paths now share one pipelining implementation. One pinned divergence from Node v26: when a response is destroyed while queued, Node assigns the destroyed message and wedges the connection untilrequestTimeout; Bun resets the connection (as the native path already did), since an HTTP/1.1 connection cannot skip a response slot.Verification
New tests fail on main and pass with this change:
test/js/node/http/node-http.test.ts: two pipelined GETs with an async handler; out-of-order completion where the queued response is written (write+end, chunked) before the first finishes; three pipelined requests answered synchronously; in-flight and queued requests/responses aborted ('close') when the connection dies; aConnection: closeresponse ends the connection and the queued response behind it is aborted, never sent; connection reset when a queued response is destroyed, with requests behind it aborted; read gate pauses dispatch once queued response bytes pass the high water mark and everything drains after release.test/js/node/http2/node-http2.test.js(allowHTTP1 ALPN fallback over real TLS): pipelined requests served in order; queued response and request aborted when the connection dies.Wire output for the ordering scenarios is byte-identical to Node v26.3.0, including header order and chunked framing.
No regressions in:
node-http.test.ts,node-http2.test.js,node-http-connect.test.ts, backpressure/pause/transfer-encoding/abort suites, and the vendoredtest-http-generic-streams,test-http-pipeline-flood,test-http-pipeline-socket-parser-typeerror,test-http-get-pipeline-problem,test-http-incoming-pipelined-socket-destroy,test-http-keep-alive-pipeline-max-requests,test-http-many-ended-pipelines,test-http-pipeline-assertionerror-finish,test-http-pipeline-outgoing-destroy,test-http-pipeline-requests-connection-leak,test-http2-https-fallback,test-http2-https-fallback-http-server-options,test-http-server-unconsume-consume.[review] gate passed · iteration 4 · 6 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 1 rejected · iteration 4
evidence per changed file