Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion src/js/node/net.ts
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,11 @@ const SocketHandlers: SocketHandler = {
if (!self || self[kclosed]) return;
self[kclosed] = true;
//socket cannot be used after close
const callback = self[kwriteCallback];
if (callback) {
self[kwriteCallback] = null;
callback(err || $ERR_SOCKET_CLOSED());
}
Comment on lines +151 to +155

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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

  1. A client socket is created via net.connect() → constructor assigns this[khandlers] = SocketHandlers2
  2. The application calls socket.write(data, callback)_write() sets self[kwriteCallback] = callback and waits for native drain
  3. The remote server closes the connection cleanly (no error) before the write completes
  4. SocketHandlers2.close fires → sets kclosed, pushes null to readable, but never invokes kwriteCallback
  5. The writable stream is stuck waiting for the callback that will never come → _final is never called → finish and close events are never emitted
  6. If using error instead of close, 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.

detachSocket(self);
SocketEmitEndNT(self, err);
self.data = null;
Expand Down Expand Up @@ -314,6 +319,11 @@ const ServerHandlers: SocketHandler<NetSocket> = {
if (!data[kclosed]) {
data[kclosed] = true;
//socket cannot be used after close
const callback = data[kwriteCallback];
if (callback) {
data[kwriteCallback] = null;
callback(err || $ERR_SOCKET_CLOSED());
}
detachSocket(data);
SocketEmitEndNT(data, err);
data.data = null;
Expand Down Expand Up @@ -540,7 +550,11 @@ const SocketHandlers2: SocketHandler<NonNullable<import("node:net").Socket["_han
if (err) $debug(err);
if (self[kclosed]) return;
self[kclosed] = true;
// TODO: should we be doing something with err?
const callback = self[kwriteCallback];
if (callback) {
self[kwriteCallback] = null;
callback(err || $ERR_SOCKET_CLOSED());
}
Comment on lines +553 to +557

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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.

Comment on lines +553 to +557

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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:

  1. this[kwriteCallback] = callback (line 1449) — stored on the socket object
  2. this.once("close", onClose) (line 1459) — where onClose (line 1452-1453) captures callback directly in a closure, NOT via kwriteCallback

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

  1. A client socket calls _write() while connecting === true → sets kwriteCallback = callback AND registers onClose listener
  2. socket.destroy() is called → _destroy (line 1115) → _handle is still set (because SocketHandlers2.close does NOT call detachSocket() — confirmed: detachSocket only appears at lines 156 and 327)
  3. _destroy calls closeSocketHandle(this, isException) (line 1145) → _handle.close() triggers the native close handler → SocketHandlers2.close fires
  4. SocketHandlers2.close (line 553-556): reads kwriteCallback, sets it to null, calls callback(err || $ERR_SOCKET_CLOSED())FIRST invocation
  5. closeSocketHandle also schedules setImmediate(() => self.emit("close", isException)) (line 2597-2599)
  6. _destroy sets _handle = null (line 1150) and calls the destroy callback (line 1153)
  7. When the setImmediate fires, self.emit("close") triggers the onClose listener from _writecallback($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_CALLBACK

Fix

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.

self[kended] = true;
if (!self.allowHalfOpen) self.write = writeAfterFIN;
self.push(null);
Expand Down
138 changes: 138 additions & 0 deletions test/regression/issue/24808.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,138 @@
import { expect, test } from "bun:test";
import { bunEnv, bunExe, tempDir } from "harness";

// https://github.com/oven-sh/bun/issues/24808
// When multiple server-side sockets have their remote clients disconnect while the server
// is actively writing data, ALL sockets should eventually emit the `close` event.
// Previously, only one socket would complete the full event lifecycle while the rest
// would stall after `end`, never emitting `close`.
test("all server sockets emit close when clients disconnect during active writes", async () => {
const NUM_CLIENTS = 4;

using dir = tempDir("24808", {
"server.js": `
const net = require('net');
const buffer = Buffer.allocUnsafeSlow(1024 * 128);
const NUM = ${NUM_CLIENTS};
let socketId = 0;
const closedSet = new Set();

const server = net.createServer((c) => {
const id = ++socketId;
let canwrite = true;

function write() {
if (!c.writable) return;
if (c.writableLength > 1024 * 1024 || !canwrite) return;
canwrite = c.write(buffer, (err) => {
if (err) return;
canwrite = true;
// Recursively write to keep the writable stream busy
write();
});
}

write();
const tt = setInterval(write, 1);
c.on("drain", write);

c.on("end", () => { clearInterval(tt); });
c.on("error", () => { clearInterval(tt); });
c.on("close", () => {
closedSet.add(id);
c.removeAllListeners("drain");
clearInterval(tt);
if (closedSet.size === NUM) {
console.log("CLOSED:" + JSON.stringify([...closedSet].sort()));
clearTimeout(failTimer);
server.close();
}
});
});

const failTimer = setTimeout(() => {
console.log("TIMEOUT");
console.log("CLOSED:" + JSON.stringify([...closedSet].sort()));
process.exit(1);
}, 10000);

server.listen(0, () => {
console.log("PORT:" + server.address().port);
});
`,
});

await using serverProc = Bun.spawn({
cmd: [bunExe(), "server.js"],
cwd: String(dir),
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});

// Read port from server stdout
const reader = serverProc.stdout.getReader();
let portLine = "";
while (true) {
const { value, done } = await reader.read();
if (done) break;
portLine += new TextDecoder().decode(value);
if (portLine.includes("\n")) break;
}

const portMatch = portLine.match(/PORT:(\d+)/);
expect(portMatch).not.toBeNull();
const port = parseInt(portMatch![1]);

// Create clients that connect, don't read data (to build up server backpressure), then disconnect
await using clientProc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`
const net = require('net');
const sockets = [];
for (let i = 0; i < ${NUM_CLIENTS}; i++) {
const s = net.connect({ port: ${port}, host: '127.0.0.1' });
s.on('error', () => {});
sockets.push(s);
}
setTimeout(() => {
sockets.forEach(s => s.destroy());
setTimeout(() => process.exit(0), 500);
}, 2000);
`,
],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});

// Wait for server to finish
let remaining = "";
while (true) {
const { value, done } = await reader.read();
if (done) break;
remaining += new TextDecoder().decode(value);
}
reader.releaseLock();

const fullOutput = portLine + remaining;

const [stdout, stderr, exitCode] = await Promise.all([
Promise.resolve(fullOutput),
serverProc.stderr.text(),
serverProc.exited,
]);
Comment on lines +122 to +126

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

Suggested change
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.


await clientProc.exited;

Comment on lines +128 to +129

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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 closedMatch = fullOutput.match(/CLOSED:(\[.*?\])/);
expect(closedMatch).not.toBeNull();

const closed = JSON.parse(closedMatch![1]);

// All sockets should have received the close event
expect(closed).toEqual([1, 2, 3, 4]);
expect(exitCode).toBe(0);
}, 15000);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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:

});