fix(net): flush pending write callback on socket close to prevent stalled streams#27161
fix(net): flush pending write callback on socket close to prevent stalled streams#27161robobun wants to merge 1 commit into
Conversation
|
✅ 4dc32 — Looks good! Reviewed 2 files across |
robobun
force-pushed
the
claude/fix-socket-close-events-24808
branch
from
4dc32bd to
665e8a3
Compare
WalkthroughNotify and invoke any pending socket write callbacks when sockets close across per-socket, per-server, and Bun.Socket close flows. Adds a regression test that reproduces concurrent writes with client disconnects to assert all sockets emit close and the process exits cleanly. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/node/net.ts (1)
547-558:⚠️ Potential issue | 🟠 MajorApply the same write callback handling to
SocketHandlers2.close.The close handler is missing the pending write callback invocation present in
SocketHandlers.close(lines 151-155) andServerHandlers.close(lines 322-326). TheSocketHandlers2.errorhandler (lines 602-606) implements this callback pattern, butSocketHandlers2.closedoes not, creating an inconsistency that could leave pending write callbacks unhandled for client-side sockets. The TODO at line 491 indicates this may be deferred pending broader refactoring.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js/node/net.ts` around lines 547 - 558, SocketHandlers2.close is missing the pending-write-callback invocation present in SocketHandlers.close and ServerHandlers.close; update SocketHandlers2.close to check socket.data.self[kwriteCallback], clear it, and invoke it (with an appropriate error or null) before returning — mirror the callback handling semantics used in SocketHandlers.close/ServerHandlers.close so pending client-side write callbacks are not left unhandled; use the same symbols self, kwriteCallback, kclosed, kended and writeAfterFIN when implementing this change.
🤖 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/24808.test.ts`:
- Around line 122-126: The Promise.all currently destructures stdout and stderr
which are never used; replace the Promise.all([...]) call so you only await the
process exit. In practice remove Promise.resolve(fullOutput) and
serverProc.stderr.text() from the array and change the destructuring to only
capture exitCode (e.g., const [exitCode] = await
Promise.all([serverProc.exited]) or simply const exitCode = await
serverProc.exited), referencing the existing symbols stdout, stderr, fullOutput,
serverProc.stderr.text(), and serverProc.exited to locate the change.
---
Outside diff comments:
In `@src/js/node/net.ts`:
- Around line 547-558: SocketHandlers2.close is missing the
pending-write-callback invocation present in SocketHandlers.close and
ServerHandlers.close; update SocketHandlers2.close to check
socket.data.self[kwriteCallback], clear it, and invoke it (with an appropriate
error or null) before returning — mirror the callback handling semantics used in
SocketHandlers.close/ServerHandlers.close so pending client-side write callbacks
are not left unhandled; use the same symbols self, kwriteCallback, kclosed,
kended and writeAfterFIN when implementing this change.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📥 Commits
Reviewing files that changed from the base of the PR and between 3e1e287 and 665e8a3d299cbbd692ab035112c3649fbb1accae.
📒 Files selected for processing (2)
src/js/node/net.tstest/regression/issue/24808.test.ts
| const [stdout, stderr, exitCode] = await Promise.all([ | ||
| Promise.resolve(fullOutput), | ||
| serverProc.stderr.text(), | ||
| serverProc.exited, | ||
| ]); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: Unused variables from Promise.all.
The stdout and stderr variables are destructured but never used. Since fullOutput is already available and used directly on line 130, the Promise.all could be simplified.
🔧 Suggested simplification
- const [stdout, stderr, exitCode] = await Promise.all([
- Promise.resolve(fullOutput),
- serverProc.stderr.text(),
- serverProc.exited,
- ]);
+ const exitCode = await serverProc.exited;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [stdout, stderr, exitCode] = await Promise.all([ | |
| Promise.resolve(fullOutput), | |
| serverProc.stderr.text(), | |
| serverProc.exited, | |
| ]); | |
| const exitCode = await serverProc.exited; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/regression/issue/24808.test.ts` around lines 122 - 126, The Promise.all
currently destructures stdout and stderr which are never used; replace the
Promise.all([...]) call so you only await the process exit. In practice remove
Promise.resolve(fullOutput) and serverProc.stderr.text() from the array and
change the destructuring to only capture exitCode (e.g., const [exitCode] =
await Promise.all([serverProc.exited]) or simply const exitCode = await
serverProc.exited), referencing the existing symbols stdout, stderr, fullOutput,
serverProc.stderr.text(), and serverProc.exited to locate the change.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| const callback = self[kwriteCallback]; | ||
| if (callback) { | ||
| self[kwriteCallback] = null; | ||
| callback(err || $ERR_SOCKET_CLOSED()); | ||
| } |
There was a problem hiding this comment.
🔴 Incomplete fix: SocketHandlers2.close (line 547) is missing the same kwriteCallback flush that this PR adds to SocketHandlers.close and ServerHandlers.close. SocketHandlers2 is the default handler for all client-initiated sockets (assigned at line 708: this[khandlers] = SocketHandlers2), so client sockets that close while a write is in-flight will stall in the same way. Its own error handler (line 602-606) already flushes kwriteCallback, confirming the pattern is needed here too.
Extended reasoning...
What the bug is
This PR fixes the issue where pending kwriteCallback is never invoked when a socket closes, causing the writable stream to stall. The fix correctly adds callback flushing to SocketHandlers.close (line 151-155) and ServerHandlers.close (line 322-326), but the same fix is missing from SocketHandlers2.close (line 547-558).
Why SocketHandlers2 matters
SocketHandlers2 is the default handler for all client-initiated sockets. In the Socket constructor at line 708, every new socket gets this[khandlers] = SocketHandlers2. This means any socket created via net.connect(), net.createConnection(), or new net.Socket().connect() uses SocketHandlers2 — making this a heavily-used code path.
The inconsistency within SocketHandlers2 itself
SocketHandlers2.error (lines 595-608) already flushes the pending write callback:
const callback = self[kwriteCallback];
if (callback) {
self[kwriteCallback] = null;
callback(error);
}But SocketHandlers2.close (lines 547-558) does not. It only sets kended = true, pushes null, and calls read(0). The existing TODO comment at line 553 (// TODO: should we be doing something with err?) even hints this was a known gap.
Step-by-step proof of the stall
- A client socket is created via
net.connect()→ constructor assignsthis[khandlers] = SocketHandlers2 - The application calls
socket.write(data, callback)→_write()setsself[kwriteCallback] = callbackand waits for native drain - The remote server closes the connection cleanly (no error) before the write completes
SocketHandlers2.closefires → setskclosed, pushes null to readable, but never invokeskwriteCallback- The writable stream is stuck waiting for the callback that will never come →
_finalis never called →finishandcloseevents are never emitted - If using
errorinstead ofclose, the callback IS flushed (line 602-606) and the stream shuts down properly — but a clean close bypasses the error handler
Impact
Any client-side net.Socket that has a pending write when the remote end closes cleanly will have its writable stream stall indefinitely. This is the exact same class of bug the PR describes and fixes for server-side sockets.
How to fix
Add the same pattern to SocketHandlers2.close, before the existing logic:
const callback = self[kwriteCallback];
if (callback) {
self[kwriteCallback] = null;
callback(err || $ERR_SOCKET_CLOSED());
}This makes all three close handlers consistent and eliminates the stall for client sockets as well.
| // All sockets should have received the close event | ||
| expect(closed).toEqual([1, 2, 3, 4]); | ||
| expect(exitCode).toBe(0); | ||
| }, 15000); |
There was a problem hiding this comment.
🟡 The test sets an explicit 15000ms timeout as the third argument to test() on line 138. The test/CLAUDE.md states: "CRITICAL: Do not set a timeout on tests. Bun already has timeouts." Remove the , 15000 argument from the test call.
Extended reasoning...
What the issue is
The new regression test at test/regression/issue/24808.test.ts passes 15000 as the third argument to the test() function on line 138:
test("all server sockets emit close when clients disconnect during active writes", async () => {
// ... test body ...
}, 15000);This sets an explicit 15000ms (15 second) timeout for the test.
Why this violates project conventions
The test/CLAUDE.md file contains an explicit rule under the "No timeouts" section (lines 118-120):
CRITICAL: Do not set a timeout on tests. Bun already has timeouts.
Bun's test runner already provides built-in timeout handling, so explicit timeouts on individual tests are unnecessary and violate the project's established conventions.
Why the timeout was likely added
The test spawns a server subprocess that does aggressive writes to 4 sockets, waits for clients to disconnect, and verifies all sockets emit close. The server itself has an internal 10-second failTimer. The 15000ms test timeout was likely added as a safety net to ensure the test doesn't hang indefinitely. However, this is exactly the kind of per-test timeout that the CLAUDE.md rule prohibits — Bun's built-in test timeout already serves this purpose.
Impact
This is a minor convention violation with no functional impact. The test works correctly with or without the explicit timeout. However, maintaining consistent adherence to project conventions keeps the test suite uniform and prevents proliferation of ad-hoc timeouts.
How to fix
Remove the , 15000 third argument from the test() call on line 138, changing:
}, 15000);to:
});
…lled streams When a native socket closes while there's a pending write callback (kwriteCallback), the writable stream gets stuck waiting for that callback to complete. This prevents the stream from proceeding through its shutdown sequence, so `finish` and `close` events are never emitted. This was most noticeable when multiple server-side sockets had their remote clients disconnect during active writes — only one socket would complete the full event lifecycle while the remaining sockets would stall after `end`. The fix invokes the pending write callback with an error in both `SocketHandlers.close` and `ServerHandlers.close`, matching what the `error` handlers already do. Closes #24808 Co-Authored-By: Claude <noreply@anthropic.com>
robobun
force-pushed
the
claude/fix-socket-close-events-24808
branch
from
665e8a3 to
69d91de
Compare
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 `@src/js/node/net.ts`:
- Around line 553-557: The write callback can be invoked twice when a socket
closes before connecting because both the close-handling branch and the
connecting-path's onClose (registered with once("close", onClose)) may call the
same self[kwriteCallback]; to fix it, make invocation idempotent by always
reading the callback into a local variable, immediately setting
self[kwriteCallback] = null before calling it, and ensure the onClose handler
uses the same pattern (or removes its listener) so it checks/clears
self[kwriteCallback] and only invokes when non-null; update both the code that
currently calls callback(err || $ERR_SOCKET_CLOSED()) and the
onClose/once("close", onClose) path to follow this guard using the
kwriteCallback symbol.
In `@test/regression/issue/24808.test.ts`:
- Around line 128-129: The test currently awaits clientProc.exited but never
verifies the result; change it to capture the exit value (e.g. const exitCode =
await clientProc.exited) and add an assertion that the exitCode equals the
expected value (typically 0) using the test/assert helper in this suite (e.g.
assert.strictEqual/expect). This targets the clientProc promise resolution so
failing client processes fail the test rather than being ignored.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📥 Commits
Reviewing files that changed from the base of the PR and between 665e8a3d299cbbd692ab035112c3649fbb1accae and 69d91de.
📒 Files selected for processing (2)
src/js/node/net.tstest/regression/issue/24808.test.ts
| const callback = self[kwriteCallback]; | ||
| if (callback) { | ||
| self[kwriteCallback] = null; | ||
| callback(err || $ERR_SOCKET_CLOSED()); | ||
| } |
There was a problem hiding this comment.
Prevent double write-callback invocation on pre-connect close.
Line 553-557 now invokes self[kwriteCallback] on close, but Line 1459 already registers a once("close", onClose) for the same callback in the connecting write path. If close happens before connect, that callback can fire twice.
🔧 Proposed fix
const callback = self[kwriteCallback];
if (callback) {
self[kwriteCallback] = null;
- callback(err || $ERR_SOCKET_CLOSED());
+ // For pre-connect writes, _write() already installs once("close", onClose)
+ // that invokes the same callback.
+ if (!self.connecting) {
+ callback(err || $ERR_SOCKET_CLOSED());
+ }
}Also applies to: 1452-1459
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/js/node/net.ts` around lines 553 - 557, The write callback can be invoked
twice when a socket closes before connecting because both the close-handling
branch and the connecting-path's onClose (registered with once("close",
onClose)) may call the same self[kwriteCallback]; to fix it, make invocation
idempotent by always reading the callback into a local variable, immediately
setting self[kwriteCallback] = null before calling it, and ensure the onClose
handler uses the same pattern (or removes its listener) so it checks/clears
self[kwriteCallback] and only invokes when non-null; update both the code that
currently calls callback(err || $ERR_SOCKET_CLOSED()) and the
onClose/once("close", onClose) path to follow this guard using the
kwriteCallback symbol.
| await clientProc.exited; | ||
|
|
There was a problem hiding this comment.
Assert the client process exit code as well.
Line 128 awaits clientProc.exited but never validates it. A client-side failure can be silently ignored and still let this test pass.
🔧 Proposed fix
- await clientProc.exited;
+ const clientExitCode = await clientProc.exited;
+ expect(clientExitCode).toBe(0);As per coding guidelines: "Always check exit codes and test error scenarios when spawning processes in tests."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/regression/issue/24808.test.ts` around lines 128 - 129, The test
currently awaits clientProc.exited but never verifies the result; change it to
capture the exit value (e.g. const exitCode = await clientProc.exited) and add
an assertion that the exitCode equals the expected value (typically 0) using the
test/assert helper in this suite (e.g. assert.strictEqual/expect). This targets
the clientProc promise resolution so failing client processes fail the test
rather than being ignored.
| const callback = self[kwriteCallback]; | ||
| if (callback) { | ||
| self[kwriteCallback] = null; | ||
| callback(err || $ERR_SOCKET_CLOSED()); | ||
| } |
There was a problem hiding this comment.
🔴 The new kwriteCallback flush in SocketHandlers2.close can invoke the write callback twice when a socket closes during the connecting phase. In _write() (lines 1448-1460), the connecting branch stores callback in kwriteCallback AND captures it directly in an onClose closure registered via this.once("close", onClose). When close fires, the flush invokes callback via kwriteCallback (first call), then closeSocketHandle schedules setImmediate(() => emit("close")) which triggers the onClose listener calling callback again from the closure (second call), since onClose does not check kwriteCallback. Fix: guard onClose by checking if (self[kwriteCallback] === callback) before invoking.
Extended reasoning...
What the bug is
The PR adds kwriteCallback flushing to SocketHandlers2.close (lines 553-556) to fix stalled streams when sockets close with pending writes. However, this introduces a double invocation of the write callback for client sockets that close during the connecting phase while a write is pending.
The two independent callback paths
In _write() (line 1448-1460), when this.connecting is true, two independent references to callback are created:
this[kwriteCallback] = callback(line 1449) — stored on the socket objectthis.once("close", onClose)(line 1459) — whereonClose(line 1452-1453) capturescallbackdirectly in a closure, NOT viakwriteCallback
The onClose listener is only removed by the "connect" event handler (line 1455-1456). If the socket never connects (destroyed before connecting), the listener persists.
Step-by-step proof of double invocation
- A client socket calls
_write()whileconnecting === true→ setskwriteCallback = callbackAND registersonCloselistener socket.destroy()is called →_destroy(line 1115) →_handleis still set (becauseSocketHandlers2.closedoes NOT calldetachSocket()— confirmed:detachSocketonly appears at lines 156 and 327)_destroycallscloseSocketHandle(this, isException)(line 1145) →_handle.close()triggers the native close handler →SocketHandlers2.closefiresSocketHandlers2.close(line 553-556): readskwriteCallback, sets it tonull, callscallback(err || $ERR_SOCKET_CLOSED())— FIRST invocationcloseSocketHandlealso schedulessetImmediate(() => self.emit("close", isException))(line 2597-2599)_destroysets_handle = null(line 1150) and calls the destroy callback (line 1153)- When the
setImmediatefires,self.emit("close")triggers theonCloselistener from_write→callback($ERR_SOCKET_CLOSED_BEFORE_CONNECTION())— SECOND invocation
Why existing safeguards do not prevent this
Nulling kwriteCallback at line 555 does NOT prevent the second invocation because onClose captures callback directly in its closure — it never reads from kwriteCallback. The onClose listener is only removed by the "connect" event handler (line 1455-1456), and since the socket never connects in this scenario, the listener persists.
Impact
In Node.js/Bun stream internals, calling the _write callback twice triggers ERR_MULTIPLE_CALLBACK (the writable stream checks an internal flag that is cleared after the first callback). This will emit an unhandled error on the socket. Trigger scenario:
const socket = net.connect({ port: 12345, host: "127.0.0.1" });
socket.write("hello"); // write while connecting
socket.destroy(); // → callback invoked twice → ERR_MULTIPLE_CALLBACKFix
Guard the onClose callback in _write by checking if kwriteCallback is still set before invoking:
function onClose() {
if (self[kwriteCallback] === callback) {
self[kwriteCallback] = null;
callback($ERR_SOCKET_CLOSED_BEFORE_CONNECTION());
}
}Since SocketHandlers2.close already sets kwriteCallback = null before calling the callback, the onClose listener would see kwriteCallback as null and skip the second invocation.
|
Please merge this. |
2 participants
Summary
finishandcloseevents when remote clients disconnect during active writeskwriteCallback) is pending, the writable stream gets stuck waiting for that callback, preventing the shutdown sequence from completingSocketHandlers.closeandServerHandlers.close, matching what theerrorhandlers already doRoot Cause
In
src/js/node/net.ts, theclosehandlers inSocketHandlersandServerHandlerswould:detachSocket()(sets_handle = null)SocketEmitEndNT()(pushes null to readable stream → firesend)But if a
_write()was in-flight (waiting for drain/completion viakwriteCallback), the callback was never invoked. This left the writable stream stuck, so Node.js stream machinery could never proceed to_final→finish→_destroy→close.The
errorhandlers already had this pattern:The
closehandlers were missing it.Test plan
test/regression/issue/24808.test.tsthat spawns a server with 4 sockets doing aggressive writes, disconnects all clients, and verifies all sockets emitcloseUSE_SYSTEM_BUN=1(reproduces the bug)bun bd test(fix works)test/js/node/net/tests show no new failuresCloses #24808
🤖 Generated with Claude Code