diff --git a/Cargo.lock b/Cargo.lock index 7b5654882f96..7b66864b0138 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1850,7 +1850,6 @@ dependencies = [ "bun_alloc", "bun_analytics", "bun_core", - "bun_crash_handler", "bun_dispatch", "bun_errno", "bun_event_loop", diff --git a/src/jsc/bindings/NoOrphansTracker.cpp b/src/jsc/bindings/NoOrphansTracker.cpp index 621e11102c57..ae000f613c1b 100644 --- a/src/jsc/bindings/NoOrphansTracker.cpp +++ b/src/jsc/bindings/NoOrphansTracker.cpp @@ -26,8 +26,8 @@ // birth records its uniqueid — is narrowed by the freeze-then-rescan loop in // `killTracked()` but cannot be fully closed from userspace. // -// All state is process-global; spawnSync is single-threaded by design (see -// Bun__currentSyncPID), so no locking. +// All state is process-global; this is only reached from the thread that +// armed the parent-death watchdog (pdeathsig::is_arming_thread), so no locking. #include "root.h" @@ -64,10 +64,7 @@ static_assert(sizeof(ProcUniqIdentifierInfo) == 56, "xnu ABI"); class NoOrphansTracker { public: - // Function-local static: lazy first-use construction, no global ctor, - // thread-safe per C++11 [stmt.dcl]. spawnSync is single-threaded anyway - // (see Bun__currentSyncPID), but this keeps the binary's static-init - // section clean. + // Function-local static keeps this out of the binary's static-init section. static NoOrphansTracker& get() { static NoOrphansTracker instance; diff --git a/src/jsc/bindings/c-bindings.cpp b/src/jsc/bindings/c-bindings.cpp index 1ead5980b9ba..62473e157c46 100644 --- a/src/jsc/bindings/c-bindings.cpp +++ b/src/jsc/bindings/c-bindings.cpp @@ -937,11 +937,14 @@ extern "C" int ffi_fileno(FILE* file) #if OS(LINUX) || OS(DARWIN) || OS(FREEBSD) #include #include +#include -// Note: We only ever use bun.spawnSync on the main thread. +// Bun.openInEditor runs spawnSync on detached threads, so register/unregister can overlap. extern "C" int64_t Bun__currentSyncPID = 0; static int Bun__pendingSignalToSend = 0; static struct sigaction previous_actions[NSIG]; +static WTF::Lock signalForwardingLock; +static int signalForwardingDepth = 0; // npm's signal list minus SIGIOT/SIGPOLL (aliases of SIGABRT/SIGIO; listing both would overwrite previous_actions[N]). // https://github.com/npm/cli/blob/fefd509992a05c2dfddbe7bc46931c42f1da69d7/workspaces/arborist/lib/signals.js#L26-L57 @@ -1004,6 +1007,10 @@ extern "C" void Bun__sendPendingSignalIfNecessary() extern "C" void Bun__registerSignalsForForwarding() { + WTF::Locker locker(signalForwardingLock); + if (signalForwardingDepth++ != 0) + return; + Bun__pendingSignalToSend = 0; struct sigaction sa; memset(&sa, 0, sizeof(sa)); @@ -1032,6 +1039,10 @@ extern "C" void Bun__unregisterSignalsForForwarding() { Bun__currentSyncPID = 0; + WTF::Locker locker(signalForwardingLock); + if (--signalForwardingDepth != 0) + return; + #define UNREGISTER_SIGNAL(SIG) \ if (sigaction(SIG, &previous_actions[SIG], NULL) == -1) { \ } diff --git a/src/spawn/Cargo.toml b/src/spawn/Cargo.toml index 38baba7ce619..355adb384d6d 100644 --- a/src/spawn/Cargo.toml +++ b/src/spawn/Cargo.toml @@ -20,7 +20,6 @@ scopeguard.workspace = true bun_analytics.workspace = true bun_dispatch.workspace = true bun_core.workspace = true -bun_crash_handler.workspace = true bun_event_loop.workspace = true bun_io.workspace = true bun_output.workspace = true diff --git a/src/spawn/lib.rs b/src/spawn/lib.rs index 4725121ebca5..ac835491fec8 100644 --- a/src/spawn/lib.rs +++ b/src/spawn/lib.rs @@ -9,8 +9,8 @@ //! `bun_runtime` re-exports them. The only non-leaf dependencies are //! `bun_io` (`FilePoll`/`KeepAlive`/`EventLoopCtx`), `bun_ptr` //! (`ThreadSafeRefCount`), `bun_io` (`BufferedWriter`), `bun_event_loop`, -//! `bun_threading`, and `bun_crash_handler` — none of which depend back on -//! this crate, so no cycle. +//! and `bun_threading` — none of which depend back on this crate, so no +//! cycle. use core::ffi::c_char; diff --git a/src/spawn/process.rs b/src/spawn/process.rs index 94b9059320cd..8d5151b55d0a 100644 --- a/src/spawn/process.rs +++ b/src/spawn/process.rs @@ -2853,8 +2853,7 @@ mod spawn_process_body { }; /// RAII guard around `Bun__registerSignalsForForwarding`: registers on - /// construction, unregisters and restores the crash-handler signal - /// disposition on drop. + /// construction, unregisters on drop. #[cfg(unix)] struct SignalForwarding; #[cfg(unix)] @@ -2869,7 +2868,6 @@ mod spawn_process_body { impl Drop for SignalForwarding { fn drop(&mut self) { Bun__unregisterSignalsForForwarding(); - bun_crash_handler::reset_on_posix(); } } diff --git a/test/js/bun/util/open-in-editor-gc.test.ts b/test/js/bun/util/open-in-editor-gc.test.ts index 715b5e9b734d..4bcf886355b0 100644 --- a/test/js/bun/util/open-in-editor-gc.test.ts +++ b/test/js/bun/util/open-in-editor-gc.test.ts @@ -152,3 +152,54 @@ test.skipIf(!isLinux)("Bun.openInEditor does not break GC signal handling", asyn await Promise.all(runs); }); + +// Each detached editor thread goes through bun.spawnSync's signal-forwarding +// register/unregister, which swaps every forwarded signal's disposition in a +// process-global table. With many threads overlapping, an unsynchronized +// table ends up restoring SIG_DFL (or the forwarding handler itself) instead +// of the handler that was installed before the burst, so a JS signal +// listener registered beforehand silently stops working or kills the process. +test.skipIf(!isLinux)("Bun.openInEditor bursts do not drop previously installed signal handlers", async () => { + const sleep = ["/usr/bin/sleep", "/bin/sleep"].find(p => existsSync(p)); + expect(sleep).toBeDefined(); + + using dir = tempDir("open-in-editor-signal-table", { + "run.js": ` + let fired = false; + process.on("SIGUSR2", () => { fired = true; }); + + let spawned = 0; + for (let i = 0; i < 64; i++) { + try { Bun.openInEditor("0.1", { editor: ${JSON.stringify(sleep)} }); spawned++; } catch {} + } + if (spawned === 0) { console.log("no editor threads spawned"); process.exit(2); } + + // The editor children each run for 100ms, so by now the burst has + // started and the forwarding handler owns SIGUSR2; the original handler + // only comes back once the last thread unregisters. Keep delivering + // until it does (or the table was corrupted and it never does). + await Bun.sleep(50); + const deadline = Date.now() + 3000; + while (!fired && Date.now() < deadline) { + process.kill(process.pid, "SIGUSR2"); + await Bun.sleep(20); + } + console.log(fired ? "ok" : "handler lost"); + `, + }); + + await using proc = Bun.spawn({ + cmd: [bunExe(), "run.js"], + env: bunEnv, + cwd: String(dir), + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + expect(stderr).toBe(""); + expect(stdout.trim()).toBe("ok"); + expect(proc.signalCode).toBeNull(); + expect(exitCode).toBe(0); +});