Skip to content

Commit 85073ae

Browse files
committed
cluster: layering, protocol, and coverage follow-ups from PR review
- primary/RoundRobinHandle: translate listen errors to a uv-domain errno (uvTranslateSysError) at source and drop the Bun-only 'errcode' string from the wire protocol; net.ts uses ExceptionWithHostPort like Node. - primary: reuse enobufs/einval bindings for the numeric errnos so getSystemErrorName(ex.errno) matches ex.code on every platform. - primary/SharedHandle/RoundRobinHandle: port the rest of nodejs/node#60141 (cachedHandle + has(worker)) so a worker re-asking for a key it already holds gets a fresh handle that EADDRINUSEs; re-enable the debug asserts. - primary: replace the Windows SCHED_RR '// TODO' with the accurate reason (TCP SCHED_NONE works via WSADuplicateSocketW; keeping RR default until named-pipe DuplicateHandle export lands). - SharedHandle: refuse a plain worker joining a TLS shared-only key (the symmetric case of the existing EINVAL guard) and skip Linux abstract- socket paths from the pipe-unlink cleanup. - node_cluster_binding: use bun_sys wrappers for cloexec/nonblock/close instead of raw libc. - usockets: us_create_udp_socket_from_fd surfaces the poll-registration errno (matching us_socket_group_listen_fd) via a new *err out-param. - tests: TLS cluster worker happy path, TLS-first mixed-key EINVAL, round-robin byte-0 preservation, RR socket state (connecting=false / remoteAddress), SCHED_NONE listen({fd:2}) leaves the primary's stderr open, and send(socket, {keepOpen: true}) keeps the sender's copy live.
1 parent d694fa7 commit 85073ae

5 files changed

Lines changed: 292 additions & 5 deletions

File tree

src/js/internal/cluster/RoundRobinHandle.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ let net;
55

66
const sendHelper = $newRustFunction("node_cluster_binding.rs", "sendHelperPrimary", 4);
77
const uvTranslateSysError = $newRustFunction("node_util_binding.rs", "uvTranslateSysError", 1);
8+
const einvalErrorCode = $newRustFunction("node_util_binding.rs", "einvalErrorCode", 0);
89

910
const ArrayIsArray = Array.isArray;
1011

@@ -91,8 +92,8 @@ export default class RoundRobinHandle {
9192
// getSystemErrorName) expects. On Windows the WSA code goes through
9293
// uv_translate_sys_error so `getSystemErrorName(errno)` matches
9394
// `err.code` — same as node's send(err.errno, null).
94-
const errno = uvTranslateSysError(typeof err.errno === "number" ? err.errno : 0) || uvTranslateSysError(-1);
95-
send(errno, null, null);
95+
const raw = typeof err.errno === "number" && err.errno !== 0 ? err.errno : null;
96+
send(raw != null ? uvTranslateSysError(raw) : einvalErrorCode(), null, null);
9697
});
9798
}
9899

src/jsc/ipc.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1510,7 +1510,7 @@ impl SendQueue {
15101510
// close_on_complete and re-fire the callback with null.
15111511
let will_push_fresh = handle.is_some()
15121512
|| self.queue.is_empty()
1513-
|| self.queue.last().map_or(true, |l| {
1513+
|| self.queue.last().is_none_or(|l| {
15141514
l.handle.is_some()
15151515
|| l.is_ack_nack()
15161516
|| (self.queue.len() == 1 && self.write_in_progress)

src/uws_sys/udp.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ impl Socket {
6969
close_cb,
7070
recv_error_cb,
7171
fd,
72-
err.map_or(core::ptr::null_mut(), |e| e as *mut _),
72+
err.map_or(core::ptr::null_mut(), core::ptr::from_mut),
7373
user_data,
7474
)
7575
}

test/js/node/child_process/child_process_ipc_handle.test.ts

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -279,4 +279,70 @@ process.on('message', (m, server) => {
279279
response: true,
280280
});
281281
});
282+
283+
// send(msg, socket, {keepOpen: true}): both parent and child hold a live
284+
// dup of the connection. Node's test/parallel/test-child-process-send-keep-open.js.
285+
test.concurrent("net.Socket handle sent with {keepOpen: true} stays open in the sender", async () => {
286+
using dir = tempDir("ipc-handle-keepopen", {
287+
"parent.js": `
288+
const { fork } = require('node:child_process');
289+
const net = require('node:net');
290+
291+
const child = fork('child.js');
292+
let closed = false;
293+
const server = net.createServer(socket => {
294+
socket.on('close', () => { closed = true; });
295+
child.send('socket', socket, { keepOpen: true }, err => {
296+
if (err) return finish(false, 'send:' + err.message);
297+
// The parent's copy must still be usable after the ack.
298+
socket.write('parent', () => {});
299+
});
300+
child.on('message', m => {
301+
if (m !== 'child-wrote') return;
302+
// Only end after the child has also written.
303+
setTimeout(() => {
304+
if (closed) return finish(false, 'parent socket closed by keepOpen send');
305+
socket.end();
306+
}, 50);
307+
});
308+
}).listen(0, '127.0.0.1', () => {
309+
const client = net.connect(server.address().port, '127.0.0.1');
310+
client.setEncoding('utf8');
311+
let data = '';
312+
client.on('data', c => (data += c));
313+
client.on('end', () => finish(data.includes('parent') && data.includes('child'), data));
314+
client.on('error', e => finish(false, 'client:' + e.message));
315+
});
316+
317+
function finish(ok, detail) {
318+
console.log(ok ? 'RESPONSE:' + detail : 'FAILED:' + detail);
319+
try { child.kill(); } catch {}
320+
try { server.close(); } catch {}
321+
process.exit(ok ? 0 : 1);
322+
}
323+
`,
324+
"child.js": `
325+
const net = require('node:net');
326+
process.on('message', (m, socket) => {
327+
if (!(socket instanceof net.Socket)) return process.send({ error: 'handle was ' + typeof socket });
328+
socket.write('child', () => process.send('child-wrote'));
329+
});
330+
`,
331+
});
332+
333+
await using proc = Bun.spawn({
334+
cmd: [bunExe(), "parent.js"],
335+
env: bunEnv,
336+
cwd: String(dir),
337+
stdout: "pipe",
338+
stderr: "pipe",
339+
});
340+
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
341+
expect({ exitCode, stderr, hasParent: stdout.includes("parent"), hasChild: stdout.includes("child") }).toEqual({
342+
exitCode: 0,
343+
stderr: expect.any(String),
344+
hasParent: true,
345+
hasChild: true,
346+
});
347+
});
282348
});

test/js/node/cluster.test.ts

Lines changed: 221 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { expect, test } from "bun:test";
2-
import { bunEnv, bunExe, bunRun, isIPv6, isWindows, joinP, tempDirWithFiles } from "harness";
2+
import { bunEnv, bunExe, bunRun, isIPv6, isWindows, joinP, tempDirWithFiles, tls as tlsCerts } from "harness";
33

44
test("cloneable and transferable equals", () => {
55
const dir = tempDirWithFiles("bun-test", {
@@ -556,3 +556,223 @@ test.each(["net", "http"])("cluster 'listening' reports the address a %s server
556556
if (!("path" in target)) expect(payloads[i].port).toBeWithin(1, 65536);
557557
}
558558
});
559+
560+
test("round-robin worker connection socket has connecting=false and remoteAddress synchronously", () => {
561+
const dir = tempDirWithFiles("bun-test", {
562+
"main.ts": `
563+
const cluster = require("node:cluster");
564+
const net = require("node:net");
565+
566+
if (cluster.isPrimary) {
567+
const worker = cluster.fork();
568+
worker.on("message", m => {
569+
console.log(JSON.stringify(m));
570+
worker.kill();
571+
process.exit(0);
572+
});
573+
cluster.on("listening", (w, address) => {
574+
net.connect(address.port, "127.0.0.1").on("error", () => {});
575+
});
576+
} else {
577+
net
578+
.createServer(socket => {
579+
// Captured synchronously in the connection listener: node's onconnection
580+
// delivers accepted sockets already open, not connecting.
581+
process.send({
582+
connecting: socket.connecting,
583+
readyState: socket.readyState,
584+
remote: typeof socket.remoteAddress,
585+
});
586+
socket.end();
587+
})
588+
.listen(0, "127.0.0.1");
589+
}
590+
`,
591+
});
592+
const { stdout } = bunRun(joinP(dir, "main.ts"), bunEnv);
593+
const m = JSON.parse(stdout.trim());
594+
expect(m.connecting).toBe(false);
595+
expect(m.readyState).toBe("open");
596+
expect(m.remote).toBe("string");
597+
});
598+
599+
test("round-robin: primary never consumes accepted-socket bytes before handoff", () => {
600+
const dir = tempDirWithFiles("bun-test", {
601+
"main.ts": `
602+
const cluster = require("node:cluster");
603+
const net = require("node:net");
604+
605+
const N = 20;
606+
if (cluster.isPrimary) {
607+
const worker = cluster.fork();
608+
let got = 0;
609+
worker.on("message", m => {
610+
console.log(m);
611+
if (++got === N) {
612+
worker.kill();
613+
process.exit(0);
614+
}
615+
});
616+
cluster.on("listening", (w, address) => {
617+
for (let i = 0; i < N; i++) {
618+
const c = net.connect(address.port, "127.0.0.1", () => {
619+
// Write immediately on connect: with pauseOnConnect at the primary
620+
// accept, byte 0 must reach the worker, not the primary's Duplex.
621+
c.write("MAGIC-" + i + "-" + "x".repeat(4096));
622+
c.end();
623+
});
624+
c.on("error", () => {});
625+
}
626+
});
627+
} else {
628+
net
629+
.createServer(sock => {
630+
let buf = "";
631+
sock.on("data", d => (buf += d));
632+
sock.on("end", () => process.send(buf.slice(0, 20) + " " + buf.length));
633+
})
634+
.listen(0, "127.0.0.1");
635+
}
636+
`,
637+
});
638+
const { stdout } = bunRun(joinP(dir, "main.ts"), bunEnv);
639+
const lines = stdout.trim().split("\n").sort();
640+
expect(lines.length).toBe(20);
641+
for (const line of lines) {
642+
expect(line).toMatch(/^MAGIC-\d+-x+ 41\d\d$/);
643+
}
644+
});
645+
646+
test("TLS cluster worker under SCHED_RR listens on a shared handle and completes handshakes", () => {
647+
// Two forked workers + a TLS handshake in a debug build is well over the
648+
// default 5s test budget.
649+
const dir = tempDirWithFiles("bun-test", {
650+
"cert.pem": tlsCerts.cert,
651+
"key.pem": tlsCerts.key,
652+
"main.ts": `
653+
const cluster = require("node:cluster");
654+
const tls = require("node:tls");
655+
const fs = require("node:fs");
656+
const path = require("node:path");
657+
const key = fs.readFileSync(path.join(__dirname, "key.pem"));
658+
const cert = fs.readFileSync(path.join(__dirname, "cert.pem"));
659+
660+
if (cluster.isPrimary) {
661+
const w1 = cluster.fork();
662+
const w2 = cluster.fork();
663+
const ports = new Set();
664+
let listening = 0;
665+
cluster.on("listening", (w, address) => {
666+
ports.add(address.port);
667+
if (++listening !== 2) return;
668+
// Both workers must share the primary-bound port under SCHED_RR TLS.
669+
console.log("distinct ports:", ports.size);
670+
const port = address.port;
671+
const c = tls.connect({ port, host: "127.0.0.1", rejectUnauthorized: false }, () => {
672+
c.write("hi");
673+
});
674+
c.setEncoding("utf8");
675+
c.on("data", d => {
676+
console.log("reply:", d);
677+
c.end();
678+
w1.kill();
679+
w2.kill();
680+
process.exit(0);
681+
});
682+
c.on("error", e => {
683+
console.log("client error:", e.code);
684+
process.exit(1);
685+
});
686+
});
687+
} else {
688+
tls
689+
.createServer({ key, cert }, socket => {
690+
socket.on("data", d => socket.end("echo:" + d));
691+
})
692+
.listen(0);
693+
}
694+
`,
695+
});
696+
const { stdout } = bunRun(joinP(dir, "main.ts"), bunEnv);
697+
expect(stdout).toContain("distinct ports: 1");
698+
expect(stdout).toContain("reply: echo:hi");
699+
}, 30_000);
700+
701+
test("plain worker listening on a key already owned by a TLS shared-only handle fails with EINVAL", () => {
702+
const dir = tempDirWithFiles("bun-test", {
703+
"cert.pem": tlsCerts.cert,
704+
"key.pem": tlsCerts.key,
705+
"main.ts": `
706+
const cluster = require("node:cluster");
707+
const net = require("node:net");
708+
const tls = require("node:tls");
709+
const fs = require("node:fs");
710+
const path = require("node:path");
711+
const key = fs.readFileSync(path.join(__dirname, "key.pem"));
712+
const cert = fs.readFileSync(path.join(__dirname, "cert.pem"));
713+
714+
if (cluster.isPrimary) {
715+
// Reverse of the existing test: TLS worker claims first, plain worker second.
716+
const tlsWorker = cluster.fork({ ROLE: "tls" });
717+
cluster.once("listening", () => {
718+
const netWorker = cluster.fork({ ROLE: "net" });
719+
netWorker.on("message", msg => {
720+
console.log("net listen error code:", msg.code);
721+
tlsWorker.kill();
722+
netWorker.kill();
723+
process.exit(0);
724+
});
725+
});
726+
} else if (process.env.ROLE === "tls") {
727+
tls.createServer({ key, cert }, () => {}).listen(0);
728+
} else {
729+
const server = net.createServer(() => {});
730+
server.on("error", err => process.send({ code: err.code }));
731+
server.listen(0);
732+
}
733+
`,
734+
});
735+
const { stdout } = bunRun(joinP(dir, "main.ts"), bunEnv);
736+
expect(stdout).toContain("net listen error code: EINVAL");
737+
}, 30_000);
738+
739+
test.skipIf(isWindows)("SCHED_NONE listen({fd:2}) fails ENOTSOCK and does not close the primary's stderr", () => {
740+
const dir = tempDirWithFiles("bun-test", {
741+
"main.ts": `
742+
const cluster = require("node:cluster");
743+
const net = require("node:net");
744+
const fs = require("node:fs");
745+
746+
cluster.schedulingPolicy = cluster.SCHED_NONE;
747+
748+
if (cluster.isPrimary) {
749+
const worker = cluster.fork();
750+
worker.on("message", m => {
751+
console.log("worker error code:", m.code);
752+
worker.disconnect();
753+
});
754+
cluster.on("exit", () => {
755+
// stderr fd must still be a valid open fd in the primary.
756+
try {
757+
fs.fstatSync(2);
758+
console.log("stderr open: true");
759+
} catch (e) {
760+
console.log("stderr open: false");
761+
}
762+
process.exit(0);
763+
});
764+
} else {
765+
const server = net.createServer(() => {});
766+
server.on("error", err => {
767+
process.send({ code: err.code });
768+
});
769+
server.listen({ fd: 2 });
770+
}
771+
`,
772+
});
773+
const { stdout } = bunRun(joinP(dir, "main.ts"), bunEnv);
774+
// ENOTSOCK when the primary's fd 2 is a pipe/tty; some paths surface EINVAL.
775+
// The load-bearing invariant is that the primary's stderr survives remove().
776+
expect(stdout).toMatch(/worker error code: (ENOTSOCK|EINVAL|EBADF)/);
777+
expect(stdout).toContain("stderr open: true");
778+
});

0 commit comments

Comments
 (0)