Repository navigation
node:http: register the detach-on-finish listener before emitting 'request' - #34488
Conversation
…quest' Middleware that replaces res.on (e.g. @polka/compression, used by vite preview) diverted the dispatcher's finish listener onto its own stream, so a kept-alive socket never cleared _httpMessage and the next request on it threw ERR_HTTP_SOCKET_ASSIGNED, killing the server. Register the listener before the 'request' event like Node.js's resOnFinish, and guard the synchronously-finished fast path with a flag instead of registering late. Fixes #34485
WalkthroughChangesThe HTTP server now prevents duplicate response detachment and pipeline advancement when responses finish synchronously. Regression tests cover sequential keep-alive requests and pipelined requests through wrapped response streams. HTTP finish handling
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/regression/issue/34485.test.ts`:
- Around line 66-70: Add coverage in the regression test around the existing
request loop for the synchronous double-advance guard: issue three genuinely
pipelined requests without awaiting each response, use synchronous res.end()
handling, then await and validate all responses arrive in request order with the
expected body. Ensure the assertions fail if the kDispatcherDetached protection
is removed, while preserving the existing response-status and body checks.
🪄 Autofix (Beta)
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: f3339c52-c36e-4c28-91ac-dd832c626f8b
📒 Files selected for processing (2)
src/js/node/_http_server.tstest/regression/issue/34485.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline test nits, I traced the listener-ordering change through the dispatcher: the sync-finish path at _http_server.ts:1076 sets kDispatcherDetached before 'finish' fires (which is async), so the guard is effective and double-advance can't happen; and pipelined responses still get the listener via the pre-emit registration at line 852, so removing it from advanceResponsePipeline is safe. One observable change worth a human eye: user 'finish' listeners now see res.socket === null (detach runs before them) — the PR says this matches Node.js, which is consistent with resOnFinish, but it's a behavior shift on the hot path.
Extended reasoning...
Checked that every path reaching server.emit('request'/'checkContinue'/'checkExpectation') first passes through the new registration at line 852, and that the removed registration in advanceResponsePipeline is fully subsumed (pipelined responses are dispatched through the same code path, so they already have the listener when they're later assigned the socket). The kDispatcherDetached guard is set synchronously after the emit returns and before any microtask/nextTick can fire 'finish', so the sync-finish branch cannot race the listener. detachSocket and advanceResponsePipeline both no-op on already-detached/empty state, so even a hypothetical double-fire would be harmless. Deferring because this is the core node:http dispatch loop with a user-observable ordering change, not because of a correctness concern.
|
Addressed the two test nits in 9b03b94 (test.concurrent, combined output assertion). On the res.socket-inside-'finish' ordering: confirmed against Node v26.3.0 that a user 'finish' listener observes res.socket === null there (resOnFinish registers first and detaches), so this change brings Bun in line rather than diverging. The node:http suites and the keep-alive/pipeline/outgoing-finish parallel tests pass with the new ordering. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/regression/issue/34485.test.ts`:
- Around line 87-95: Update both combined output assertions in the regression
test to stop requiring stderr to equal an empty string, while retaining stderr
in the structured result. Continue asserting the exact stdout and exitCode
values in each assertion.
🪄 Autofix (Beta)
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: 8d797642-f36e-4dc5-a2a1-0781a9b70498
📒 Files selected for processing (1)
test/regression/issue/34485.test.ts
There was a problem hiding this comment.
Thanks for addressing both test nits in 9b03b94. I didn't find any bugs, but this reorders internal 'finish' listener registration in the node:http dispatch path (with a user-observable change: user 'finish' listeners now see res.socket === null, per the Node v26.3.0 check) and removes the advance-time registration in advanceResponsePipeline, so it's worth a human look at the pipeline/sync-finish interaction.
What was reviewed:
- Listener registration at
_http_server.ts:852is before both'request'emit sites (1055/1064), andemitResponseFinishHandleSocketstill runs beforedetachSocket. kDispatcherDetachedguard vs. the removedadvanceResponsePipelineregistration — pipelined responses return before the sync-finish branch, so the pre-registered listener covers both paths without double-advance.emitAsyncResponseFinishon upgrade / no-socket paths —socket != nullgate andadvanceResponsePipeline's missing-socket bail make the always-on registration harmless.- Regression tests: both fail on main with
ERR_HTTP_SOCKET_ASSIGNED, nowtest.concurrentwith combined-object assertions.
Extended reasoning...
Overview
The PR moves the emitAsyncResponseFinish 'finish' listener registration from after the 'request' event (and from inside advanceResponsePipeline for queued responses) to a single site before 'request' is emitted, alongside the existing emitResponseFinishHandleSocket registration. A new kDispatcherDetached flag is set on the synchronous-finish path so the now-always-registered listener does not detach/advance a second time. Two regression tests exercise keep-alive and pipelined requests through @polka/compression-style middleware that replaces res.on.
Security risks
None identified. This is internal listener-ordering in the node:http compat layer; no auth, crypto, or untrusted-input parsing is touched.
Level of scrutiny
High. src/js/node/_http_server.ts is the core node:http server dispatch path — every node-compat HTTP server in Bun runs through it. The change is small in line count but subtle in semantics: it removes two registration sites in favor of one, relies on a new guard flag to keep the sync-finish path idempotent, and changes the relative order of internal vs. user 'finish' listeners (user listeners now observe res.socket === null, which robobun verified matches Node v26.3.0). The reasoning in the PR description and the robobun follow-up on why the kDispatcherDetached guard is not independently testable is sound, but the interaction between the pipelined-return branch, the sync-finish branch, and the removed advanceResponsePipeline registration is the kind of thing a maintainer familiar with the #32488 rewrite should confirm.
Other factors
All prior review feedback is addressed and resolved: CodeRabbit's pipelined-coverage request (1cc2159), and my two nits on test.concurrent and combined {stdout, stderr, exitCode} assertions (9b03b94). The evidence block shows the tests fail on unpatched main with the exact reported error and pass with the fix. Node parallel test-http-pipeline-*, test-http-keep-alive*, and test-http-outgoing-finish* suites reportedly pass. No bugs surfaced from the bug-hunting pass this run.
There was a problem hiding this comment.
No issues found — the fix traces correctly and both prior nits are addressed. Deferring to a human only because this reorders the internal 'finish' listeners on the core node:http dispatch path (user 'finish' listeners now run after detach), which is production-critical enough to warrant a maintainer glance.
Checked: the pre-registered emitAsyncResponseFinish is reached for all three paths (async, sync-finished, queued pipelined) since it's added before the isPipelined branch; the removed registration in advanceResponsePipeline would otherwise double-register. 'finish' is emitted via process.nextTick (_http_server.ts:3236/3250), so kDispatcherDetached is always set before the listener can observe it — no window for a double-advance. detachSocket is idempotent (guarded on socket._httpMessage === this). The relative order of emitResponseFinishHandleSocket → emitAsyncResponseFinish is preserved, so the keep-alive-timeout / pipelined-queue guards in onResponseFinishHandleSocket see the same state as before.
Extended reasoning...
Overview
The PR moves the registration of emitAsyncResponseFinish (the detach-socket + advance-pipeline listener) from after the 'request' event to before it, so middleware that monkeypatches res.on inside the handler (as @polka/compression does) cannot divert the internal listener onto a different emitter. To keep the synchronously-finished fast path from double-detaching, a new kDispatcherDetached symbol is set on the response before the inline detach, and the listener bails on it. The now-redundant registration inside advanceResponsePipeline is removed. Two subprocess regression tests cover keep-alive reuse and true pipelining through the offending middleware shape.
Correctness trace
- All responses reach the new registration: line 852 is above the
isPipelinedbranching (line 922) and the sync-finished check (line 1076), so everyServerResponsecreated by the dispatcher gets the listener exactly once. The removed line inadvanceResponsePipelineis therefore necessary to avoid double-registration for queued responses. - Sync-detach ordering:
res.end()schedules'finish'viaprocess.nextTick(_http_server.ts:3236,3250); nothing betweenserver.emit('request', ...)and line 1079 drains the tick queue (onlysocket.cork()runs), sokDispatcherDetached = trueis always set before the listener fires. The guard is thus effective, and its author-stated untestability (mutually exclusive with a non-empty pipeline queue) checks out. - Idempotence: even if the guard were absent,
detachSocketcheckssocket._httpMessage === thisandadvanceResponsePipelinebails on an empty queue, so the sync path would still be a no-op — the guard is belt-and-suspenders, matching the author's characterization. - Listener order:
emitResponseFinishHandleSocket(line 848) still runs beforeemitAsyncResponseFinish(line 852), soonResponseFinishHandleSocket'ssocket._httpMessage !== resandkPipelinedResponses?.lengthguards observe the same pre-detach state as on main. - Observable change: user
'finish'listeners now run after detach (sores.socket === null), whereas on main they ran between the two internal listeners. The author verified this matches Node v26.3.0'sresOnFinish, and the node:http compat layer's job is to match Node — this is a deliberate alignment, not a regression.
Security risks
None. Pure JS listener-ordering in the node:http compat layer; no auth, crypto, parsing, or untrusted-input handling.
Level of scrutiny
High — this is the per-request dispatch path for every node:http server on Bun, and it changes when the socket is detached relative to user listeners. The change itself is small and well-reasoned, but the blast radius is every keep-alive and pipelined connection.
Other factors
- Evidence block shows the tests fail on main (ASAN debug) with the exact
ERR_HTTP_SOCKET_ASSIGNEDand pass with the fix; release build already passes on main (documented as debug-only regression from #32488). - Both prior inline nits (test.concurrent, combined-object assertion) were addressed in 9b03b94; the CodeRabbit follow-up on
stderr: ""was reasonably declined (bunEnv + load-bearing crash signal) and withdrawn. - Node parallel suites for pipeline/keep-alive/outgoing-finish reportedly pass; the author also verified the original
vite previewrepro end-to-end.
Given the critical path, deferring for a maintainer glance rather than auto-approving.
|
CI status (latest: build 74751 at bd5d926): the changes in this PR pass on all lanes, including both regression tests. The red lanes across runs are unrelated: JSC assertion SIGABRTs in worker_threads tests on the debian-13 x64-asan lane (test-worker-message-port-transfer-terminate.js, then test-http2-reset-flood.js; both also tracked as main breaks, and the http2 one passes 25/25 locally under this branch's ASAN build), plus retry-passed flakes on Windows/macOS lanes this diff does not touch. The diff is ready. |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
The placement fix is right — registering before emit('request') is exactly what Node does, and it's why middleware that hijacks res.on can't swallow the listener. Two things to change before this lands.
1. Merge the two 'finish' listeners into one
Node registers exactly one 'finish' listener (lib/_http_server.js:1326-1329, before the emit('request') at L1371/L1383):
res.on('finish', resOnFinish.bind(undefined, req, res, socket, state, server));resOnFinish does all of it in one function: shift state.incoming → req._dump() → res.detachSocket(socket) → emitCloseNT → then _last ? destroySoon() : state.outgoing.length === 0 ? setTimeout(keepAliveTimeout) : outgoing.shift().assignSocket(socket).
This PR ends up with two always-registered listeners, which costs a per-request allocation. Our EventEmitter always stores an array, so the cost isn't at registration — it's in emit:
// src/js/node/events.ts:175
const maybeClonedHandlers = handlers.length > 1 ? handlers.slice() : handlers;Going 1 → 2 listeners drops off that no-clone fast path, so every response's 'finish' emit allocates a throwaway array. Measured in isolation (construct + register + emit):
listeners on finish |
ns/emit |
|---|---|
| 1 | 4.9 |
| 2 | 9.2 |
~4.4ns/request. Against a ~6µs request that's ~0.07% — it will not show up on a hello-world bench, so this is not a blocker on magnitude. But there's no reason to pay it: the split is an artifact of emitAsyncResponseFinish previously being registered lazily, not a requirement. Hoisting it should have merged it:
function emitResponseFinish() {
const socket = this.req?.socket ?? this.socket;
onResponseFinishHandleSocket(socket?.server, socket, this);
if (this[kDispatcherDetached]) return;
if (socket != null) this.detachSocket(socket);
advanceResponsePipeline(socket?.server, socket);
}Keep the current order — socket-handling half before detach. That's inverted relative to Node, but onResponseFinishHandleSocket's socket._httpMessage !== res and kPipelinedResponses?.length checks only work pre-detach, so don't "fix" it to match Node here.
2. kDispatcherDetached is order-dependent on finish being deferred — say so
http_res[kDispatcherDetached] = true is assigned at _http_server.ts:1079, i.e. after the request handler has already run. On the face of it the guard is set too late: a handler that calls res.end() synchronously would emit 'finish', run the detach-and-advance listener, and only then reach the assignment — detaching and advancing the pipeline twice.
It's correct only because ServerResponse.end always emits 'finish' from a process.nextTick (_http_server.ts:3249), never synchronously, and nothing between emit('request') and L1079 drains the tick queue. That's load-bearing and entirely non-obvious from the code at either site. Please add a comment at the assignment pointing at the deferred emit — otherwise someone makes end() emit synchronously for a fast path later and this breaks silently, in the pipelining path, where it's miserable to debug.
Checked, no action needed
Dropping the res.on("finish", emitAsyncResponseFinish) from advanceResponsePipeline is safe: a queued response's end() goes through bufferPipelinedEnd, which pushes the op and sets finished = true without emitting 'finish', so a queued response can't advance the pipeline before it owns the socket.
|
Adding to point 1 above — the second listener isn't only a cost, it's observable from user code, and today we match Node exactly.
This PR makes it The part that actually bites is the
So a handler that attaches 9 That moves the merge from "nice, saves 4ns" to "required for compat". One listener, please. |
One listener keeps res.listenerCount('finish') === 1 like Node (the
internal listener counts against the user's MaxListeners budget) and
stays on the EventEmitter one-handler emit fast path. Also document
that kDispatcherDetached relies on end() deferring the 'finish' emit
to process.nextTick, at both sites.
|
Updated 4:32 PM PT - Jul 17th, 2026
❌ @robobun, your commit bd5d926 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 34488That installs a local version of the PR into your bun-34488 --bun |
|
Both done in 22424f8.
Re-verified: both regression tests pass (and still fail on main), node-http.test.ts at 131 pass with only the pre-existing env-dependent proxy failure, and the pipeline/keep-alive/outgoing-finish parallel suites all pass. |
2 participants
What does this PR do?
Fixes #34485:
vite preview(and any server using@polka/compression-style middleware) crashed on the second keep-alive request withRegression from the node:http rewrite (#32488); Bun 1.3.14 is unaffected.
Repro (no vite needed):
Serve two requests over one keep-alive connection with
Accept-Encoding: gzipand the server process dies on the second one.Cause: the dispatcher registered its detach-on-finish listener (
emitAsyncResponseFinish, which clearssocket._httpMessageand advances the response pipeline) after emitting'request'. Middleware that replacesres.onduring the handler, like@polka/compression, diverted that registration onto its gzip stream. The listener then fired on the zlib stream with noreq/socket,detachSocketnever ran, and the kept-alive socket kept a stale_httpMessage, so the next request on it threw inassignSocketInternaland the uncaught error killed the server.Fix: register the listener before emitting
'request', like Node.js'sresOnFinish, so user code cannot swallow it. The synchronously-finished fast path (which detaches inline) now sets a flag the listener checks, instead of skipping registration, so the pipeline is not advanced twice. The advance-time registration inadvanceResponsePipelineis removed for the same reason; queued pipelined responses had the same exposure. This also matches Node.js in thatres.socketis alreadynullinside user'finish'listeners.How did you verify your code works?
test/regression/issue/34485.test.ts: fails on main with the exactERR_HTTP_SOCKET_ASSIGNEDcrash, passes with the fix (verified viagit stash push -- src/+ rebuild).bun create viteproject served byvite previewunder the debug build survives 50 iterations of browser-style keep-alive asset loads (previously crashed on iteration 1), same for a minimal@polka/compressionserver.bun bd test test/js/node/http/passes except 4 failures that fail identically on unmodified main (env-dependent proxy test, debug-build timeouts).test-http-pipeline-*,test-http-outgoing-finish*,test-http-keep-alive*,test-http-keepalive-request,test-http-response-closepass.[review] gate passed · iteration 4 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 1 rejected · iteration 4
evidence per changed file
root cause · written by the author bot
The root cause was that each request registered a fresh 'finish' listener on the response, and when middleware wrapped the response stream or the finish event fired synchronously, the dispatcher could detach and advance the keep-alive pipeline twice, so the next pipelined request attempted to assign a socket to a response that was already bound, raising ERR_HTTP_SOCKET_ASSIGNED. The fix consolidates the finish handling into a single emitResponseFinish listener, tracks synchronous detachment with a kDispatcherDetached flag, and defers the finish emission via process.nextTick so detachment an…