From 507dabacfaa2d81111d17a3ed50c202a6650aeae Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 16 Sep 2026 14:04:39 +0000 Subject: [PATCH 1/3] spawn: let the waiter thread and process.on("SIGCHLD") share SIGCHLD On Linux without pidfd, the waiter thread and a JS SIGCHLD listener each call sigaction(SIGCHLD) and replace the handler of the other. A listener added after the first spawn left the waiter thread without a wakeup, so proc.exited never resolved. A listener added before it never fired. The waiter thread's handler now also forwards the signal to the JS listeners. BunProcess.cpp reports each change of a signal disposition for JS listeners through Bun__onSignalDispositionChanged. For SIGCHLD, the waiter thread then installs its handler again and checks its children once. --- src/jsc/PosixSignalHandle.rs | 15 ++++++ src/jsc/bindings/BunProcess.cpp | 7 +++ src/spawn/process.rs | 46 +++++++++++++++- .../spawn/spawn-sigchld-listener-fixture.ts | 53 +++++++++++++++++++ test/js/bun/spawn/spawn.test.ts | 35 ++++++++++++ 5 files changed, 154 insertions(+), 2 deletions(-) create mode 100644 test/js/bun/spawn/spawn-sigchld-listener-fixture.ts diff --git a/src/jsc/PosixSignalHandle.rs b/src/jsc/PosixSignalHandle.rs index 0a2042e0bf8c..4ec7ab1903c7 100644 --- a/src/jsc/PosixSignalHandle.rs +++ b/src/jsc/PosixSignalHandle.rs @@ -152,6 +152,21 @@ pub(crate) extern "C" fn Bun__onSignalListenerCountChanged(number: i32, count: i } } +/// C++ `onDidChangeListeners` calls this after it set the disposition of `number` for the +/// first `process.on()` listener, and after it restored the disposition for the +/// removal of the last one (main-thread VM only). Native code that needs the same signal +/// takes the disposition back here. +#[cfg(unix)] +#[unsafe(no_mangle)] +pub(crate) extern "C" fn Bun__onSignalDispositionChanged(number: i32, has_listeners: bool) { + #[cfg(any(target_os = "linux", target_os = "android"))] + if number == libc::SIGCHLD { + bun_spawn::process::WaiterThread::set_js_listens_for_sigchld(has_listeners); + } + #[cfg(not(any(target_os = "linux", target_os = "android")))] + let _ = (number, has_listeners); +} + /// Watcher-thread query: only ever true for `bun run --watch` (the count is /// mirrored solely when `WATCH_MODE_KILL_SIGNAL` is set). pub fn watch_kill_signal_has_listeners() -> bool { diff --git a/src/jsc/bindings/BunProcess.cpp b/src/jsc/bindings/BunProcess.cpp index 4deedb2a8b8f..effee8cfb7c7 100644 --- a/src/jsc/bindings/BunProcess.cpp +++ b/src/jsc/bindings/BunProcess.cpp @@ -1541,6 +1541,11 @@ extern "C" void Bun__ensureSignalHandler(); extern "C" bool Bun__isMainThreadVM(); extern "C" void Bun__onPosixSignal(int signalNumber); extern "C" void Bun__onSignalListenerCountChanged(int signalNumber, int listenerCount); +#if !OS(WINDOWS) +// Call this after the disposition of a signal changed for its JS listeners. Native code that +// needs the same signal (the spawn waiter thread needs SIGCHLD) takes the disposition back there. +extern "C" void Bun__onSignalDispositionChanged(int signalNumber, bool hasListeners); +#endif __attribute__((noinline)) static void forwardSignal(int signalNumber) { @@ -1653,6 +1658,7 @@ static void onDidChangeListeners(EventEmitter& eventEmitter, const Identifier& e #if !OS(WINDOWS) Bun__ensureSignalHandler(); installForwardSignalHandler(signalNumber); + Bun__onSignalDispositionChanged(signalNumber, true); #else signal_handle.handle = Bun__UVSignalHandle__init( eventEmitter.scriptExecutionContext()->jsGlobalObject(), @@ -1676,6 +1682,7 @@ static void onDidChangeListeners(EventEmitter& eventEmitter, const Identifier& e // Don't uninstall the old handler if it's not the one we installed. signal(signalNumber, oldHandler); } + Bun__onSignalDispositionChanged(signalNumber, false); #else SignalHandleValue signal_handle = signalToContextIdsMap->get(signalNumber); Bun__UVSignalHandle__close(signal_handle.handle); diff --git a/src/spawn/process.rs b/src/spawn/process.rs index bb4f39e62940..a9c06114d00c 100644 --- a/src/spawn/process.rs +++ b/src/spawn/process.rs @@ -997,6 +997,8 @@ pub mod waiter_thread_posix { use bun_event_loop::ConcurrentTask::{ConcurrentTask, Task, TaskTag}; use bun_event_loop::task_tag; use bun_threading::UnboundedQueue; + #[cfg(any(target_os = "linux", target_os = "android"))] + use core::sync::atomic::AtomicBool; pub struct WaiterThreadPosix { pub(crate) started: AtomicU32, @@ -1368,6 +1370,9 @@ pub mod waiter_thread_posix { #[cfg(any(target_os = "linux", target_os = "android"))] { + // Before the sigaction: a JS listener change that replaces `wakeup` after it + // must see the flag, so that it installs `wakeup` again. + HANDLES_SIGCHLD.store(true, Ordering::SeqCst); // SAFETY: sigaction with a valid handler. unsafe { let mut current_mask: libc::sigset_t = bun_core::ffi::zeroed(); @@ -1383,6 +1388,18 @@ pub mod waiter_thread_posix { } } } + + /// `process.on("SIGCHLD")` got its first listener or lost its last one, and the + /// caller has set the SIGCHLD disposition for that (main thread). + #[cfg(any(target_os = "linux", target_os = "android"))] + pub fn set_js_listens_for_sigchld(listens: bool) { + JS_LISTENS_FOR_SIGCHLD.store(listens, Ordering::SeqCst); + if HANDLES_SIGCHLD.load(Ordering::SeqCst) { + Self::reload_handlers(); + // A child that exited while `wakeup` was not the handler did not wake the thread. + wake(); + } + } } pub(crate) fn init() -> Result<(), std::io::Error> { @@ -1418,13 +1435,38 @@ pub mod waiter_thread_posix { Ok(()) } + /// `wakeup` is the SIGCHLD handler, or the waiter thread is about to install it. + #[cfg(any(target_os = "linux", target_os = "android"))] + static HANDLES_SIGCHLD: AtomicBool = AtomicBool::new(false); + + /// `process.on("SIGCHLD")` has a listener. SIGCHLD has one disposition, so + /// `wakeup` also does the work of the handler that the listener installed. + #[cfg(any(target_os = "linux", target_os = "android"))] + static JS_LISTENS_FOR_SIGCHLD: AtomicBool = AtomicBool::new(false); + + #[cfg(any(target_os = "linux", target_os = "android"))] + unsafe extern "C" { + /// `bun_jsc` (PosixSignalHandle.rs): queues the signal for the `process.on()` + /// listeners. Async-signal-safe. + safe fn Bun__onPosixSignal(number: c_int); + } + + /// Makes the waiter thread call `wait4` for each process again. #[cfg(any(target_os = "linux", target_os = "android"))] - extern "C" fn wakeup(_: c_int) { + fn wake() { let one: [u8; 8] = (1usize).to_ne_bytes(); - // eventfd is write-once in init() before this handler is installed. + // eventfd is write-once in init() before the waiter thread starts. let _ = bun_sys::write(instance_ref().eventfd, &one).unwrap_or(0); } + #[cfg(any(target_os = "linux", target_os = "android"))] + extern "C" fn wakeup(signal: c_int) { + wake(); + if JS_LISTENS_FOR_SIGCHLD.load(Ordering::SeqCst) { + Bun__onPosixSignal(signal); + } + } + pub(crate) fn loop_() { // SAFETY: NUL-terminated literal. Output::Source::configure_named_thread(bun_core::ZStr::from_static(b"Waitpid\0")); diff --git a/test/js/bun/spawn/spawn-sigchld-listener-fixture.ts b/test/js/bun/spawn/spawn-sigchld-listener-fixture.ts new file mode 100644 index 000000000000..ebfcc0d7a791 --- /dev/null +++ b/test/js/bun/spawn/spawn-sigchld-listener-fixture.ts @@ -0,0 +1,53 @@ +// SIGCHLD has one disposition. process.on("SIGCHLD") needs it, and with +// BUN_FEATURE_FLAG_FORCE_WAITER_THREAD the waiter thread needs it too: SIGCHLD is what +// tells that thread to call wait4() again. Each one must keep working when the other one +// starts or stops. +// +// argv[2] is "before" or "after": when the listener is added, relative to the first spawn. +// The first spawn starts the waiter thread. Prints one line per child exit. +import { spawn } from "bun"; + +let signals = 0; +let waiting: { count: number; resolve: () => void } | undefined; + +function onSIGCHLD() { + signals++; + if (waiting && signals >= waiting.count) waiting.resolve(); +} + +function signalCount(count: number) { + const { promise, resolve } = Promise.withResolvers(); + waiting = { count, resolve }; + if (signals >= count) resolve(); + return promise; +} + +// The children exit one at a time, so each exit is one SIGCHLD. +let expectedSignals = 0; + +async function childExit(child: string, listening: boolean) { + const proc = spawn({ cmd: ["cat"], stdin: "pipe", stdout: "pipe", stderr: "inherit" }); + + // The echo shows that the child runs. The waiter thread called wait4() for it when it was + // spawned, and now sleeps. Only SIGCHLD wakes it for the exit. + proc.stdin.write("x"); + await proc.stdin.flush(); + await proc.stdout.getReader().read(); + + await proc.stdin.end(); + const exitCode = await proc.exited; + if (listening) await signalCount(++expectedSignals); + + console.log(JSON.stringify({ child, exitCode, signals })); +} + +const order = process.argv[2]; + +if (order === "before") process.on("SIGCHLD", onSIGCHLD); +await childExit("first spawn", order === "before"); + +if (order === "after") process.on("SIGCHLD", onSIGCHLD); +await childExit("second spawn", true); + +process.off("SIGCHLD", onSIGCHLD); +await childExit("listener removed", false); diff --git a/test/js/bun/spawn/spawn.test.ts b/test/js/bun/spawn/spawn.test.ts index a9003d595c48..24ad09cf11fe 100644 --- a/test/js/bun/spawn/spawn.test.ts +++ b/test/js/bun/spawn/spawn.test.ts @@ -609,6 +609,41 @@ for (let [gcTick, label] of [ }); } +// SIGCHLD has one disposition. process.on("SIGCHLD") needs it, and the waiter thread needs it +// to learn that a child exited. The first spawn starts the waiter thread. +describe.skipIf(Boolean(process.env.BUN_FEATURE_FLAG_FORCE_WAITER_THREAD) || (!isLinux && !isAndroid))( + "a SIGCHLD listener and Bun.spawn both see each child exit", + () => { + const waiterThread = { "BUN_FEATURE_FLAG_FORCE_WAITER_THREAD": "1", "BUN_GARBAGE_COLLECTOR_LEVEL": "1" }; + // Number of listener calls after each of the three child exits. The listener is removed before the third. + const signals = { before: [1, 2, 2], after: [0, 1, 1] }; + + // Not concurrent: a fixture that waits forever is only killed on the timeout of a serial test. + it.each([ + ["waiter thread, listener added before the first spawn", waiterThread, "before"], + ["waiter thread, listener added after the first spawn", waiterThread, "after"], + ["pidfd, listener added before the first spawn", {}, "before"], + ["pidfd, listener added after the first spawn", {}, "after"], + ] as const)("%s", async (_, env, order) => { + await using proc = spawn({ + cmd: [bunExe(), join(import.meta.dir, "spawn-sigchld-listener-fixture.ts"), order], + env: { ...bunEnv, ...env }, + stdin: "ignore", + stdout: "pipe", + stderr: "inherit", + }); + const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]); + const lines = stdout.split("\n").filter(Boolean); + expect(lines.map(line => JSON.parse(line))).toEqual([ + { child: "first spawn", exitCode: 0, signals: signals[order][0] }, + { child: "second spawn", exitCode: 0, signals: signals[order][1] }, + { child: "listener removed", exitCode: 0, signals: signals[order][2] }, + ]); + expect(exitCode).toBe(0); + }); + }, +); + // The waiter thread is the Linux fallback for kernels/sandboxes without pidfd; // kqueue platforms (macOS, FreeBSD) always have EVFILT_PROC and its non-Linux // loop has no wakeup for processes appended after it starts. From 127bab949f474e1120ac583c25ea2fd9b85a899a Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 16 Sep 2026 14:43:21 +0000 Subject: [PATCH 2/3] spawn: tell the waiter thread about a SIGCHLD listener before the disposition changes The listener flag is now stored from Bun__onSignalListenerCountChanged, which runs before BunProcess.cpp installs forwardSignal. The waiter thread's handler can replace forwardSignal at any time after that, and it then already forwards to the listener. While a JS listener exists, the waiter thread installs its handler without SA_NOCLDSTOP. The listener then also hears a stopped and a continued child, as it does without the waiter thread. A lock orders the flag read and the sigaction of the two threads that can install. --- src/jsc/PosixSignalHandle.rs | 11 +++++--- src/jsc/bindings/BunProcess.cpp | 10 +++++--- src/spawn/process.rs | 25 ++++++++++++++++--- .../spawn/spawn-sigchld-listener-fixture.ts | 15 ++++++++--- test/js/bun/spawn/spawn.test.ts | 5 ++-- 5 files changed, 51 insertions(+), 15 deletions(-) diff --git a/src/jsc/PosixSignalHandle.rs b/src/jsc/PosixSignalHandle.rs index 4ec7ab1903c7..71189d049614 100644 --- a/src/jsc/PosixSignalHandle.rs +++ b/src/jsc/PosixSignalHandle.rs @@ -137,6 +137,11 @@ static WATCH_SIGINT_LISTENERS: AtomicU32 = AtomicU32::new(0); /// count change here (main-thread VM only, platform signal numbers). #[unsafe(no_mangle)] pub(crate) extern "C" fn Bun__onSignalListenerCountChanged(number: i32, count: i32) { + // The SIGCHLD handler of the spawn waiter thread forwards to the JS listeners. + #[cfg(any(target_os = "linux", target_os = "android"))] + if number == libc::SIGCHLD { + bun_spawn::process::WaiterThread::set_js_listens_for_sigchld(count > 0); + } let watch_signal = i32::from(WATCH_MODE_KILL_SIGNAL.load(Ordering::Relaxed)); if watch_signal == 0 { return; @@ -158,13 +163,13 @@ pub(crate) extern "C" fn Bun__onSignalListenerCountChanged(number: i32, count: i /// takes the disposition back here. #[cfg(unix)] #[unsafe(no_mangle)] -pub(crate) extern "C" fn Bun__onSignalDispositionChanged(number: i32, has_listeners: bool) { +pub(crate) extern "C" fn Bun__onSignalDispositionChanged(number: i32) { #[cfg(any(target_os = "linux", target_os = "android"))] if number == libc::SIGCHLD { - bun_spawn::process::WaiterThread::set_js_listens_for_sigchld(has_listeners); + bun_spawn::process::WaiterThread::on_sigchld_disposition_changed(); } #[cfg(not(any(target_os = "linux", target_os = "android")))] - let _ = (number, has_listeners); + let _ = number; } /// Watcher-thread query: only ever true for `bun run --watch` (the count is diff --git a/src/jsc/bindings/BunProcess.cpp b/src/jsc/bindings/BunProcess.cpp index effee8cfb7c7..3a7d0c960195 100644 --- a/src/jsc/bindings/BunProcess.cpp +++ b/src/jsc/bindings/BunProcess.cpp @@ -1544,7 +1544,7 @@ extern "C" void Bun__onSignalListenerCountChanged(int signalNumber, int listener #if !OS(WINDOWS) // Call this after the disposition of a signal changed for its JS listeners. Native code that // needs the same signal (the spawn waiter thread needs SIGCHLD) takes the disposition back there. -extern "C" void Bun__onSignalDispositionChanged(int signalNumber, bool hasListeners); +extern "C" void Bun__onSignalDispositionChanged(int signalNumber); #endif __attribute__((noinline)) static void forwardSignal(int signalNumber) @@ -1632,7 +1632,9 @@ static void onDidChangeListeners(EventEmitter& eventEmitter, const Identifier& e if (auto signalNumber = signalNameToNumberMap->get(eventName.string())) { int listenerCount = eventEmitter.listenerCount(eventName); - // Mirror the count for the watcher thread's --watch-kill-signal check. + // Mirror the count for the watcher thread's --watch-kill-signal check, and for the + // spawn waiter thread's SIGCHLD handler. Keep this before the disposition changes below: + // that handler can replace forwardSignal at any time and must know the listener by then. Bun__onSignalListenerCountChanged(signalNumber, listenerCount); #if OS(LINUX) // SIGKILL and SIGSTOP cannot be handled, and JSC needs its own signal handler to @@ -1658,7 +1660,7 @@ static void onDidChangeListeners(EventEmitter& eventEmitter, const Identifier& e #if !OS(WINDOWS) Bun__ensureSignalHandler(); installForwardSignalHandler(signalNumber); - Bun__onSignalDispositionChanged(signalNumber, true); + Bun__onSignalDispositionChanged(signalNumber); #else signal_handle.handle = Bun__UVSignalHandle__init( eventEmitter.scriptExecutionContext()->jsGlobalObject(), @@ -1682,7 +1684,7 @@ static void onDidChangeListeners(EventEmitter& eventEmitter, const Identifier& e // Don't uninstall the old handler if it's not the one we installed. signal(signalNumber, oldHandler); } - Bun__onSignalDispositionChanged(signalNumber, false); + Bun__onSignalDispositionChanged(signalNumber); #else SignalHandleValue signal_handle = signalToContextIdsMap->get(signalNumber); Bun__UVSignalHandle__close(signal_handle.handle); diff --git a/src/spawn/process.rs b/src/spawn/process.rs index a9c06114d00c..bfcc60801016 100644 --- a/src/spawn/process.rs +++ b/src/spawn/process.rs @@ -1373,27 +1373,43 @@ pub mod waiter_thread_posix { // Before the sigaction: a JS listener change that replaces `wakeup` after it // must see the flag, so that it installs `wakeup` again. HANDLES_SIGCHLD.store(true, Ordering::SeqCst); + // The JS thread and the waiter thread can both be here. With the lock, the + // last sigaction has the flags for the last value of `JS_LISTENS_FOR_SIGCHLD`. + let _lock = RELOAD_HANDLERS_LOCK.lock(); + let js_listens = JS_LISTENS_FOR_SIGCHLD.load(Ordering::SeqCst); // SAFETY: sigaction with a valid handler. unsafe { let mut current_mask: libc::sigset_t = bun_core::ffi::zeroed(); libc::sigemptyset(&raw mut current_mask); libc::sigaddset(&raw mut current_mask, libc::SIGCHLD); - let act = libc::sigaction { + let mut act = libc::sigaction { sa_sigaction: wakeup as *const () as usize, sa_mask: current_mask, sa_flags: libc::SA_NOCLDSTOP, sa_restorer: None, }; + if js_listens { + // A JS listener also hears a stopped or a continued child, as it + // does without the waiter thread. + act.sa_flags &= !libc::SA_NOCLDSTOP; + } libc::sigaction(libc::SIGCHLD, &raw const act, core::ptr::null_mut()); } } } - /// `process.on("SIGCHLD")` got its first listener or lost its last one, and the - /// caller has set the SIGCHLD disposition for that (main thread). + /// `process.on("SIGCHLD")` has a listener, or has none any more (main thread). Call + /// this before the SIGCHLD disposition changes for that, so that `wakeup` never + /// runs for a listener it does not know. #[cfg(any(target_os = "linux", target_os = "android"))] pub fn set_js_listens_for_sigchld(listens: bool) { JS_LISTENS_FOR_SIGCHLD.store(listens, Ordering::SeqCst); + } + + /// The caller has set the SIGCHLD disposition for the first JS listener, or for the + /// removal of the last one (main thread). `wakeup` takes SIGCHLD back. + #[cfg(any(target_os = "linux", target_os = "android"))] + pub fn on_sigchld_disposition_changed() { if HANDLES_SIGCHLD.load(Ordering::SeqCst) { Self::reload_handlers(); // A child that exited while `wakeup` was not the handler did not wake the thread. @@ -1444,6 +1460,9 @@ pub mod waiter_thread_posix { #[cfg(any(target_os = "linux", target_os = "android"))] static JS_LISTENS_FOR_SIGCHLD: AtomicBool = AtomicBool::new(false); + #[cfg(any(target_os = "linux", target_os = "android"))] + static RELOAD_HANDLERS_LOCK: bun_threading::Guarded<()> = bun_threading::Guarded::new(()); + #[cfg(any(target_os = "linux", target_os = "android"))] unsafe extern "C" { /// `bun_jsc` (PosixSignalHandle.rs): queues the signal for the `process.on()` diff --git a/test/js/bun/spawn/spawn-sigchld-listener-fixture.ts b/test/js/bun/spawn/spawn-sigchld-listener-fixture.ts index ebfcc0d7a791..98ba0c8a8bea 100644 --- a/test/js/bun/spawn/spawn-sigchld-listener-fixture.ts +++ b/test/js/bun/spawn/spawn-sigchld-listener-fixture.ts @@ -22,10 +22,11 @@ function signalCount(count: number) { return promise; } -// The children exit one at a time, so each exit is one SIGCHLD. +// One child changes state at a time, and the listener hears each change before the next +// one. So each change is one SIGCHLD. let expectedSignals = 0; -async function childExit(child: string, listening: boolean) { +async function childExit(child: string, listening: boolean, stopAndContinue = false) { const proc = spawn({ cmd: ["cat"], stdin: "pipe", stdout: "pipe", stderr: "inherit" }); // The echo shows that the child runs. The waiter thread called wait4() for it when it was @@ -34,6 +35,14 @@ async function childExit(child: string, listening: boolean) { await proc.stdin.flush(); await proc.stdout.getReader().read(); + if (stopAndContinue) { + // A listener also hears a child that stops and a child that continues. + proc.kill("SIGSTOP"); + await signalCount(++expectedSignals); + proc.kill("SIGCONT"); + await signalCount(++expectedSignals); + } + await proc.stdin.end(); const exitCode = await proc.exited; if (listening) await signalCount(++expectedSignals); @@ -47,7 +56,7 @@ if (order === "before") process.on("SIGCHLD", onSIGCHLD); await childExit("first spawn", order === "before"); if (order === "after") process.on("SIGCHLD", onSIGCHLD); -await childExit("second spawn", true); +await childExit("second spawn", true, true); process.off("SIGCHLD", onSIGCHLD); await childExit("listener removed", false); diff --git a/test/js/bun/spawn/spawn.test.ts b/test/js/bun/spawn/spawn.test.ts index 24ad09cf11fe..a6854001209b 100644 --- a/test/js/bun/spawn/spawn.test.ts +++ b/test/js/bun/spawn/spawn.test.ts @@ -615,8 +615,9 @@ describe.skipIf(Boolean(process.env.BUN_FEATURE_FLAG_FORCE_WAITER_THREAD) || (!i "a SIGCHLD listener and Bun.spawn both see each child exit", () => { const waiterThread = { "BUN_FEATURE_FLAG_FORCE_WAITER_THREAD": "1", "BUN_GARBAGE_COLLECTOR_LEVEL": "1" }; - // Number of listener calls after each of the three child exits. The listener is removed before the third. - const signals = { before: [1, 2, 2], after: [0, 1, 1] }; + // Number of listener calls after each of the three child exits. The second child also stops + // and continues, which is one call each. The listener is removed before the third child. + const signals = { before: [1, 4, 4], after: [0, 3, 3] }; // Not concurrent: a fixture that waits forever is only killed on the timeout of a serial test. it.each([ From b89093bf0a0d658b0c0232995598d4c2ba389020 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 16 Sep 2026 14:53:31 +0000 Subject: [PATCH 3/3] spawn: shorten the comments of the SIGCHLD handler change --- src/jsc/PosixSignalHandle.rs | 5 +---- src/jsc/bindings/BunProcess.cpp | 8 +++----- src/spawn/process.rs | 22 +++++++--------------- 3 files changed, 11 insertions(+), 24 deletions(-) diff --git a/src/jsc/PosixSignalHandle.rs b/src/jsc/PosixSignalHandle.rs index 71189d049614..7e5073ef2157 100644 --- a/src/jsc/PosixSignalHandle.rs +++ b/src/jsc/PosixSignalHandle.rs @@ -157,10 +157,7 @@ pub(crate) extern "C" fn Bun__onSignalListenerCountChanged(number: i32, count: i } } -/// C++ `onDidChangeListeners` calls this after it set the disposition of `number` for the -/// first `process.on()` listener, and after it restored the disposition for the -/// removal of the last one (main-thread VM only). Native code that needs the same signal -/// takes the disposition back here. +/// C++ `onDidChangeListeners` changed the disposition of `number` for JS listeners. Native users of it take it back. #[cfg(unix)] #[unsafe(no_mangle)] pub(crate) extern "C" fn Bun__onSignalDispositionChanged(number: i32) { diff --git a/src/jsc/bindings/BunProcess.cpp b/src/jsc/bindings/BunProcess.cpp index 3a7d0c960195..8967b1d9bfd1 100644 --- a/src/jsc/bindings/BunProcess.cpp +++ b/src/jsc/bindings/BunProcess.cpp @@ -1542,8 +1542,7 @@ extern "C" bool Bun__isMainThreadVM(); extern "C" void Bun__onPosixSignal(int signalNumber); extern "C" void Bun__onSignalListenerCountChanged(int signalNumber, int listenerCount); #if !OS(WINDOWS) -// Call this after the disposition of a signal changed for its JS listeners. Native code that -// needs the same signal (the spawn waiter thread needs SIGCHLD) takes the disposition back there. +// Call it after the disposition of a signal changed for JS listeners. Native users of that signal take it back. extern "C" void Bun__onSignalDispositionChanged(int signalNumber); #endif @@ -1632,9 +1631,8 @@ static void onDidChangeListeners(EventEmitter& eventEmitter, const Identifier& e if (auto signalNumber = signalNameToNumberMap->get(eventName.string())) { int listenerCount = eventEmitter.listenerCount(eventName); - // Mirror the count for the watcher thread's --watch-kill-signal check, and for the - // spawn waiter thread's SIGCHLD handler. Keep this before the disposition changes below: - // that handler can replace forwardSignal at any time and must know the listener by then. + // Mirror the count for the watcher thread's --watch-kill-signal check. + // Keep it before the disposition changes below: the spawn waiter thread's SIGCHLD handler reads it. Bun__onSignalListenerCountChanged(signalNumber, listenerCount); #if OS(LINUX) // SIGKILL and SIGSTOP cannot be handled, and JSC needs its own signal handler to diff --git a/src/spawn/process.rs b/src/spawn/process.rs index bfcc60801016..af55ba37ab9b 100644 --- a/src/spawn/process.rs +++ b/src/spawn/process.rs @@ -1370,11 +1370,9 @@ pub mod waiter_thread_posix { #[cfg(any(target_os = "linux", target_os = "android"))] { - // Before the sigaction: a JS listener change that replaces `wakeup` after it - // must see the flag, so that it installs `wakeup` again. + // Set before the sigaction: a JS listener change after it must install `wakeup` again. HANDLES_SIGCHLD.store(true, Ordering::SeqCst); - // The JS thread and the waiter thread can both be here. With the lock, the - // last sigaction has the flags for the last value of `JS_LISTENS_FOR_SIGCHLD`. + // Two threads can be here. The lock makes the last sigaction use the last flag value. let _lock = RELOAD_HANDLERS_LOCK.lock(); let js_listens = JS_LISTENS_FOR_SIGCHLD.load(Ordering::SeqCst); // SAFETY: sigaction with a valid handler. @@ -1389,8 +1387,7 @@ pub mod waiter_thread_posix { sa_restorer: None, }; if js_listens { - // A JS listener also hears a stopped or a continued child, as it - // does without the waiter thread. + // A JS listener also hears a stopped or continued child, as with pidfd. act.sa_flags &= !libc::SA_NOCLDSTOP; } libc::sigaction(libc::SIGCHLD, &raw const act, core::ptr::null_mut()); @@ -1398,16 +1395,13 @@ pub mod waiter_thread_posix { } } - /// `process.on("SIGCHLD")` has a listener, or has none any more (main thread). Call - /// this before the SIGCHLD disposition changes for that, so that `wakeup` never - /// runs for a listener it does not know. + /// Main thread. Call it before the SIGCHLD disposition changes for a JS listener. #[cfg(any(target_os = "linux", target_os = "android"))] pub fn set_js_listens_for_sigchld(listens: bool) { JS_LISTENS_FOR_SIGCHLD.store(listens, Ordering::SeqCst); } - /// The caller has set the SIGCHLD disposition for the first JS listener, or for the - /// removal of the last one (main thread). `wakeup` takes SIGCHLD back. + /// Main thread. The SIGCHLD disposition changed for a JS listener: `wakeup` takes it back. #[cfg(any(target_os = "linux", target_os = "android"))] pub fn on_sigchld_disposition_changed() { if HANDLES_SIGCHLD.load(Ordering::SeqCst) { @@ -1455,8 +1449,7 @@ pub mod waiter_thread_posix { #[cfg(any(target_os = "linux", target_os = "android"))] static HANDLES_SIGCHLD: AtomicBool = AtomicBool::new(false); - /// `process.on("SIGCHLD")` has a listener. SIGCHLD has one disposition, so - /// `wakeup` also does the work of the handler that the listener installed. + /// `process.on("SIGCHLD")` has a listener. SIGCHLD has one disposition, so `wakeup` forwards to it. #[cfg(any(target_os = "linux", target_os = "android"))] static JS_LISTENS_FOR_SIGCHLD: AtomicBool = AtomicBool::new(false); @@ -1465,8 +1458,7 @@ pub mod waiter_thread_posix { #[cfg(any(target_os = "linux", target_os = "android"))] unsafe extern "C" { - /// `bun_jsc` (PosixSignalHandle.rs): queues the signal for the `process.on()` - /// listeners. Async-signal-safe. + /// `bun_jsc`: queues the signal for the `process.on()` listeners. Async-signal-safe. safe fn Bun__onPosixSignal(number: c_int); }