fix(runtime): Restore default signal handler after user handlers are unregistered (#22757)

<!--
Before submitting a PR, please read
https://docs.deno.com/runtime/manual/references/contributing

1. Give the PR a descriptive title.

  Examples of good title:
    - fix(std/http): Fix race condition in server
    - docs(console): Update docstrings
    - feat(doc): Handle nested reexports

  Examples of bad title:
    - fix #7123
    - update docs
    - fix bugs

2. Ensure there is a related issue and it is referenced in the PR text.
3. Ensure there are tests that cover the changes.
4. Ensure `cargo test` passes.
5. Ensure `./tools/format.js` passes without changing files.
6. Ensure `./tools/lint.js` passes.
7. Open as a draft PR if your work is still in progress. The CI won't
run
   all steps, but you can add '[ci]' to a commit message to force it to.
8. If you would like to run the benchmarks on the CI, add the 'ci-bench'
label.
-->

Fixes #22724. Fixes #7164.

This does add a dependency on `signal-hook`, but it's just a higher
level API on top of `signal-hook-registry` (which we and `tokio` already
depend on) and doesn't add any transitive deps.

Cargo.lock

@@ -87,9 +87,9 @@ dependencies = [
 
 [[package]]
 name = "ahash"
-version = "0.8.6"
+version = "0.8.11"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "91429305e9f0a25f6205c5b8e0d2db09e0708a7a6df0f42212bb56c32c8ac97a"
+checksum = "e89da841a80418a9b391ebaea17f5c112ffaaa96f621d2c285b5174da76b9011"
 dependencies = [
  "cfg-if",
  "getrandom",
@@ -1781,6 +1781,7 @@ dependencies = [
  "ring",
  "rustyline",
  "serde",
+ "signal-hook",
  "signal-hook-registry",
  "test_server",
  "tokio",
@@ -5662,6 +5663,16 @@ version = "0.1.5"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "45bb67a18fa91266cc7807181f62f9178a6873bfad7dc788c42e6430db40184f"
 
+[[package]]
+name = "signal-hook"
+version = "0.3.17"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "8621587d4798caf8eb44879d42e56b9a93ea5dcd315a6487c357130095b62801"
+dependencies = [
+ "libc",
+ "signal-hook-registry",
+]
+
 [[package]]
 name = "signal-hook-registry"
 version = "1.4.1"

runtime/Cargo.toml

@@ -119,6 +119,7 @@ regex.workspace = true
 ring.workspace = true
 rustyline = { workspace = true, features = ["custom-bindings"] }
 serde.workspace = true
+signal-hook = "0.3.17"
 signal-hook-registry = "1.4.0"
 tokio.workspace = true
 tokio-metrics.workspace = true

runtime/ops/signal.rs

@@ -12,7 +12,13 @@ use deno_core::ResourceId;
 
 use std::borrow::Cow;
 use std::cell::RefCell;
+#[cfg(unix)]
+use std::collections::BTreeMap;
 use std::rc::Rc;
+#[cfg(unix)]
+use std::sync::atomic::AtomicBool;
+#[cfg(unix)]
+use std::sync::Arc;
 
 #[cfg(unix)]
 use tokio::signal::unix::signal;
@@ -32,13 +38,52 @@ use tokio::signal::windows::CtrlC;
 deno_core::extension!(
   deno_signal,
   ops = [op_signal_bind, op_signal_unbind, op_signal_poll],
+  state = |state| {
+    #[cfg(unix)]
+    {
+      state.put(SignalState::default());
+    }
+  }
 );
 
+#[cfg(unix)]
+#[derive(Default)]
+struct SignalState {
+  enable_default_handlers: BTreeMap<libc::c_int, Arc<AtomicBool>>,
+}
+
+#[cfg(unix)]
+impl SignalState {
+  /// Disable the default signal handler for the given signal.
+  ///
+  /// Returns the shared flag to enable the default handler later, and whether a default handler already existed.
+  fn disable_default_handler(
+    &mut self,
+    signo: libc::c_int,
+  ) -> (Arc<AtomicBool>, bool) {
+    use std::collections::btree_map::Entry;
+
+    match self.enable_default_handlers.entry(signo) {
+      Entry::Occupied(entry) => {
+        let enable = entry.get();
+        enable.store(false, std::sync::atomic::Ordering::Release);
+        (enable.clone(), true)
+      }
+      Entry::Vacant(entry) => {
+        let enable = Arc::new(AtomicBool::new(false));
+        entry.insert(enable.clone());
+        (enable, false)
+      }
+    }
+  }
+}
+
 #[cfg(unix)]
 /// The resource for signal stream.
 /// The second element is the waker of polling future.
 struct SignalStreamResource {
   signal: AsyncRefCell<Signal>,
+  enable_default_handler: Arc<AtomicBool>,
   cancel: CancelHandle,
 }
 
@@ -548,11 +593,29 @@ fn op_signal_bind(
       "Binding to signal '{sig}' is not allowed",
     )));
   }
+
+  let signal = AsyncRefCell::new(signal(SignalKind::from_raw(signo))?);
+
+  let (enable_default_handler, has_default_handler) = state
+    .borrow_mut::<SignalState>()
+    .disable_default_handler(signo);
+
   let resource = SignalStreamResource {
-    signal: AsyncRefCell::new(signal(SignalKind::from_raw(signo))?),
+    signal,
     cancel: Default::default(),
+    enable_default_handler: enable_default_handler.clone(),
   };
   let rid = state.resource_table.add(resource);
+
+  if !has_default_handler {
+    // restore default signal handler when the signal is unbound
+    // this can error if the signal is not supported, if so let's just leave it as is
+    let _ = signal_hook::flag::register_conditional_default(
+      signo,
+      enable_default_handler,
+    );
+  }
+
   Ok(rid)
 }
 
@@ -606,6 +669,15 @@ pub fn op_signal_unbind(
   state: &mut OpState,
   #[smi] rid: ResourceId,
 ) -> Result<(), AnyError> {
-  state.resource_table.take_any(rid)?.close();
+  let resource = state.resource_table.take::<SignalStreamResource>(rid)?;
+
+  #[cfg(unix)]
+  {
+    resource
+      .enable_default_handler
+      .store(true, std::sync::atomic::Ordering::Release);
+  }
+
+  resource.close();
   Ok(())
 }

tests/unit/signal_test.ts

@@ -172,10 +172,20 @@ Deno.test(
     }
 
     // Sends SIGUSR1 (irrelevant signal) 3 times.
+    // By default SIGUSR1 terminates, so set it to a no-op for this test.
+    let count = 0;
+    const irrelevant = () => {
+      count++;
+    };
+    Deno.addSignalListener("SIGUSR1", irrelevant);
     for (const _ of Array(3)) {
       await delay(20);
       Deno.kill(Deno.pid, "SIGUSR1");
     }
+    while (count < 3) {
+      await delay(20);
+    }
+    Deno.removeSignalListener("SIGUSR1", irrelevant);
 
     // No change
     assertEquals(c, "010101000");

tests/unit_node/process_test.ts

@@ -237,7 +237,7 @@ Deno.test({
 
 Deno.test({
   name: "process.off signal",
-  ignore: true, // This test fails to terminate
+  ignore: Deno.build.os == "windows",
   async fn() {
     const testTimeout = setTimeout(() => fail("Test timed out"), 10_000);
     try {
@@ -246,13 +246,13 @@ Deno.test({
           "eval",
           `
           import process from "node:process";
-          console.log("ready");
           setInterval(() => {}, 1000);
           const listener = () => {
+            process.off("SIGINT", listener);
             console.log("foo");
-            process.off("SIGINT", listener)
           };
           process.on("SIGINT", listener);
+          console.log("ready");
           `,
         ],
         stdout: "piped",
@@ -275,6 +275,7 @@ Deno.test({
       while (!output.includes("foo\n")) {
         await delay(10);
       }
+      process.kill("SIGINT");
       await process.status;
     } finally {
       clearTimeout(testTimeout);