Skip to content
Open
Show file tree
Hide file tree
Changes from 8 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
17 changes: 15 additions & 2 deletions src/jsc/bindings/c-bindings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -937,11 +937,14 @@ extern "C" int ffi_fileno(FILE* file)
#if OS(LINUX) || OS(DARWIN) || OS(FREEBSD)
#include <signal.h>
#include <pthread.h>
#include <mutex>

// Note: We only ever use bun.spawnSync on the main thread.
// Bun.openInEditor runs spawnSync on detached threads, so register/unregister can overlap.
Comment thread
claude[bot] marked this conversation as resolved.
extern "C" int64_t Bun__currentSyncPID = 0;
static int Bun__pendingSignalToSend = 0;
static struct sigaction previous_actions[NSIG];
static std::mutex signalForwardingLock;
static int signalForwardingDepth = 0;
Comment thread
claude[bot] marked this conversation as resolved.
Outdated

// 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
Expand Down Expand Up @@ -1004,6 +1007,10 @@ extern "C" void Bun__sendPendingSignalIfNecessary()

extern "C" void Bun__registerSignalsForForwarding()
{
std::lock_guard<std::mutex> lock(signalForwardingLock);
if (signalForwardingDepth++ != 0)
return;

Bun__pendingSignalToSend = 0;
struct sigaction sa;
memset(&sa, 0, sizeof(sa));
Expand All @@ -1028,17 +1035,23 @@ extern "C" void Bun__registerSignalsForForwarding()
#undef REGISTER_SIGNAL
}

extern "C" void Bun__unregisterSignalsForForwarding()
// Returns true when this was the outermost caller and the previous dispositions were restored.
extern "C" bool Bun__unregisterSignalsForForwarding()
{
Bun__currentSyncPID = 0;

std::lock_guard<std::mutex> lock(signalForwardingLock);
if (--signalForwardingDepth != 0)
return false;

#define UNREGISTER_SIGNAL(SIG) \
if (sigaction(SIG, &previous_actions[SIG], NULL) == -1) { \
}

FOR_EACH_SIGNAL(UNREGISTER_SIGNAL)
memset(previous_actions, 0, sizeof(previous_actions));
#undef UNREGISTER_SIGNAL
return true;
}

#endif
Expand Down
5 changes: 3 additions & 2 deletions src/spawn/process.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2868,8 +2868,9 @@ mod spawn_process_body {
#[cfg(unix)]
impl Drop for SignalForwarding {
fn drop(&mut self) {
Bun__unregisterSignalsForForwarding();
bun_crash_handler::reset_on_posix();
if Bun__unregisterSignalsForForwarding() {
bun_crash_handler::reset_on_posix();
}
}
Comment thread
claude[bot] marked this conversation as resolved.
}

Expand Down
4 changes: 3 additions & 1 deletion src/spawn_sys/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,9 @@ pub mod ffi {
/// Install SIGINT/SIGTERM/… handlers that record the signal for
/// forwarding to [`Bun__currentSyncPID`].
pub safe fn Bun__registerSignalsForForwarding();
pub safe fn Bun__unregisterSignalsForForwarding();
/// Returns true when this was the outermost call and the previous
/// dispositions were restored.

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.

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

pub safe fn Bun__unregisterSignalsForForwarding() -> bool;

// macOS p_puniqueid descendant tracker — see NoOrphansTracker.cpp.
pub safe fn Bun__noOrphans_begin(kq: c_int, root: pid_t);
Expand Down
47 changes: 47 additions & 0 deletions test/js/bun/util/open-in-editor-gc.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -152,3 +152,50 @@ 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); }

// Let every detached thread finish its sleep and unregister.
await Bun.sleep(500);
Comment thread
claude[bot] marked this conversation as resolved.
Outdated

process.kill(process.pid, "SIGUSR2");
const deadline = Date.now() + 2000;
while (!fired && Date.now() < deadline) await Bun.sleep(5);
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);
});
Loading