http: emit ServerResponse 'close' on client disconnect#29220
http: emit ServerResponse 'close' on client disconnect#29220
Conversation
|
Updated 7:05 PM PT - Apr 12th, 2026
❌ @robobun, your commit 5bccdd4 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 29220That installs a local version of the PR into your bun-29220 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughInvoke the stored close callback from NodeHTTPServerSocket when the underlying handle closes so ServerResponse "close" listeners run on client disconnect; add two regression tests verifying ServerResponse emits "close" when a TCP client aborts mid-response or before any write. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/29219.test.ts`:
- Around line 2-6: The header multi-line prose describing the bug should be
trimmed to a single-line GitHub issue URL comment in the test file's top header;
remove the detailed multi-line explanation and leave only a single-line comment
with the issue URL (i.e., collapse the existing header comment block into one
line containing the issue link) so regression tests under
test/regression/issue/*.test.ts contain just the standard issue URL line.
🪄 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: a1005972-3f10-48f3-bc66-e8cdca757d0b
📒 Files selected for processing (2)
src/js/node/_http_server.tstest/regression/issue/29219.test.ts
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
When the canUseInternalAssignSocket fast path is taken,
assignSocketInternal stores onServerResponseClose on the socket via
setCloseCallback (socket[kCloseCallback] = ...) instead of registering
a real EventEmitter listener. Nothing was invoking that stored
callback when the socket's underlying handle closed, so
ServerResponse.on('close', ...) never ran on client abort.
Fire callCloseCallback(this) at the end of NodeHTTPServerSocket#onClose
after the request destroy, mirroring what Node's socket.on('close')
listener does. This covers both the mid-response case (#29219) and
the no-write case (#14697).
Fixes #29219
Fixes #14697
robobun
force-pushed
the
farm/127b442d/fix-server-response-close-on-abort
branch
from
9044d39 to
6e4fcf0
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/js/node/_http_server.ts:1109-1118— Whenres.destroy()is called, the'close'event fires twice on theServerResponse: once synchronously fromdestroy()'s directthis.emit('close'), and once asynchronously from the newNodeHTTPServerSocket.emithook added by this PR. Fix: inServerResponse.prototype.destroy(), setthis._closed = truebefore callingthis.emit('close'), or callthis.detachSocket(this.socket)to clear the socket back-reference before emitting.Extended reasoning...
What the bug is and how it manifests
ServerResponse.prototype.destroy()emits'close'twice when called explicitly: once synchronously (via the directthis.emit('close')call indestroy()) and a second time asynchronously (via the newNodeHTTPServerSocket.emithook added by this PR that routes the socket's'close'event back to the attachedServerResponse). Anyres.on('close', handler)listener will therefore fire twice instead of once.The specific code path that triggers it
ServerResponse.prototype.destroy()(lines ~1626-1636) setsthis.destroyed = true, callsthis?.socket?.destroy()(scheduling async socket teardown), then callsthis.emit('close')synchronously.ServerResponse.prototype.emitintercepts'close'and callscallCloseCallback(this), which invokes and clearsres[kCloseCallback]— but never setsres._closed = true. That flag is only set insideemitCloseNT, whichdestroy()bypasses entirely.destroy()also never callsdetachSocket(), sosocket._httpMessagestill points toresandsocket[kCloseCallback]still holdsonServerResponseClose.- Asynchronously, the socket's underlying handle is torn down and the
Duplexbase class emits'close'on the socket. - The new
NodeHTTPServerSocket.emitoverride (added by this PR, lines 1109-1118) catches'close'and callscallCloseCallback(socket). socket[kCloseCallback]is stillonServerResponseClose, which inspectssocket._httpMessage(stillres) and callsemitCloseNT(res).emitCloseNTguards withif (\!self._closed)— but since step 2 never set_closed, this isfalse— so it sets_closed = trueand callsres.emit('close')a second time.
Why existing code doesn't prevent it
The
_closedguard inemitCloseNTexists precisely to prevent double emissions, but it only works whenemitCloseNTis the sole emitter.destroy()short-circuits that path with a directthis.emit('close')call and relies on no second trigger ever arriving — which was true before this PR, since the socket's'close'event never reached the response. This PR's new socket hook breaks that assumption.What the impact would be
Any caller of
res.destroy()— including error handlers, thecaptureRejectionSymbolpath (line ~372), or user code that aborts a response — will observe theirres.on('close')listener firing twice. This is an observable regression from pre-PR behavior and can cause double-cleanup, double-rejection, or resource-counting errors in downstream code.Step-by-step proof
const http = require('http'); let closeCount = 0; const server = http.createServer((req, res) => { res.on('close', () => closeCount++); res.destroy(); // Tick 0 (sync): // destroy() -> this.emit('close') -> ServerResponse.emit -> // callCloseCallback(res) [clears res[kCloseCallback], does NOT set _closed] // -> Stream.prototype.emit fires listeners -> closeCount = 1 // Tick N (async, after socket._destroy completes): // Duplex emits 'close' on socket // -> NodeHTTPServerSocket.emit hook -> callCloseCallback(socket) // -> socket[kCloseCallback]() = onServerResponseClose() // -> socket._httpMessage is still res (detachSocket never called) // -> emitCloseNT(res): res._closed is false -> fires 'close' again // -> closeCount = 2 ← BUG });
How to fix it
In
ServerResponse.prototype.destroy(), addthis._closed = true;beforethis.emit('close')so that the downstreamemitCloseNTguard catches the duplicate. Alternatively, callthis.detachSocket(this.socket)before emitting to sever bothsocket._httpMessageandsocket[kCloseCallback], preventingonServerResponseClosefrom reaching the response at all.
|
The double-emit analysis targets the first commit (9044d39 — socket const server = http.createServer((req, res) => {
let n = 0; res.on("close", () => { console.log("close #" + ++n); });
res.destroy();
});Prints |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/29219.test.ts`:
- Around line 72-79: The test uses setTimeout to wait before calling
client.destroy which makes it flaky; instead create and await a "request-seen"
signal resolved from the server's request/connection handler and call
client.destroy only after that promise resolves. Locate the test block around
server.listen and net.createConnection and replace the setTimeout usage with a
Promise that is resolved inside the server's request handler (or
server.on("connection")/server.on("request") callback), then await that promise
before invoking client.destroy so the disconnect happens only after the server
has entered the handler.
- Around line 23-38: The test is waiting on an unreliable client disconnect
signal: replace the graceful half-close call client.end() with an abrupt close
via client.destroy(), and stop relying on req.on("error"); instead resolve the
surrounding promise from the response's "close" event (use the res.close
handler) and verify the response was cut off by checking res.writableEnded ===
false before resolving (keep the existing server/connection setup and adjust
handlers around client and res accordingly).
🪄 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: f2c193fa-75b4-4e38-8cb1-3f7d4326d127
📒 Files selected for processing (2)
src/js/node/_http_server.tstest/regression/issue/29219.test.ts
Address coderabbit review:
- First test resolves from res.on('close') and asserts
res.writableEnded === false, instead of relying on req.on('error').
Uses client.destroy() for an abrupt abort.
- Second test awaits a request-seen signal before calling
client.destroy(), replacing the setTimeout(100) tick hack.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/29219.test.ts`:
- Around line 77-79: The test currently contains a tautological assertion
expect(true).toBe(true) after awaiting Promise.all([resClose, reqClose]); remove
that no-op assertion and either leave the awaited Promise.all([resClose,
reqClose]) as the verification or replace it with a concrete assertion about the
closure outcome (for example assert the resolved value or a socket/connection
closed flag associated with resClose/reqClose). Update the block around resClose
and reqClose so the test verifies the actual signal (remove
expect(true).toBe(true) or replace it with an assertion referencing the
Promise.all result or the relevant closed-state variables).
🪄 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: 7653fb2d-a7fe-41cb-82bc-2e6716ff1cab
📒 Files selected for processing (1)
test/regression/issue/29219.test.ts
Replace expect(true).toBe(true) with a concrete assertion on the
resolved value from res.on('close').
|
CI noise unrelated to this PR:
All 55+ other steps passed on both builds. The regression test for #29219 / #14697 passes on every platform it ran on. |
|
Third consecutive unrelated flake on this PR:
None of these touch |
1 participant
Summary
http.ServerResponsenever fired its'close'event when the client aborted mid-response. Reported in #29219; same root cause as #14697.Repro
Node / Deno:
Bun, pre-fix:
Cause
assignSocketInternaltakes thecanUseInternalAssignSocketfast path and storesonServerResponseCloseon the socket viasetCloseCallback(i.e.socket[kCloseCallback] = onServerResponseClose) instead of registering a real.once('close', ...)listener — this is a ~10% perf win in JSC.When the underlying native handle was closed (via
handle.onclose),NodeHTTPServerSocket#onClosedestroyed theIncomingMessagebut never invoked the storedkCloseCallback. TheonServerResponseClose→emitCloseNT(httpMessage)chain that forwards the event to the attachedServerResponsetherefore never ran.Fix
Call
callCloseCallback(this)at the end ofNodeHTTPServerSocket#onClose, after the request-destroy step. This runs whether the handle was closed by the native side (abort) or via#onCloseForDestroy, so both the mid-response case (#29219) and the no-write case (#14697) are covered.Tests
test/regression/issue/29219.test.tscovers two scenarios:ServerResponse "close"event on client disconnect during active HTTP response #29219 repro).Verified with the gate:
resClosed: false, second test times out)Fixes #29219
Fixes #14697