Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Signal default actions never restored after sig.dispose() called #7164

Closed
kevinkassimo opened this issue Aug 23, 2020 · 5 comments · Fixed by #22757
Closed

Signal default actions never restored after sig.dispose() called #7164

kevinkassimo opened this issue Aug 23, 2020 · 5 comments · Fixed by #22757
Labels
bug Something isn't working correctly runtime Relates to code in the runtime crate

Comments

@kevinkassimo
Copy link
Contributor

kevinkassimo commented Aug 23, 2020

Example:

const sig = Deno.signal(Deno.Signal.SIGINT);
// don't exit
const exitBlocker = setTimeout(() => {}, 100000);
await sig;
sig.dispose()
console.log("Signal disposed");
// Now if you try Ctrl-C it will still never exit the program, as default is never restored.

This is due to upstream Tokio behavior: See https://github.com/tokio-rs/tokio/blob/138eef352671aadffca304d62c69cb2582e64f2a/tokio/src/signal/unix.rs#L346-L361

It is debatable if we should reset to SIG_DFL ourselves, but it will break Tokio's handling due to Tokio having no idea we secretly resets and it relies on the presence of some SigInfo struct to determine if we have ever registered for listeners, meaning future reregistration will no longer work.

While it is probably more of a Tokio issue here if we want the default handling back, it might be worthy for us to discuss even if reset to default is the right thing to do. Also we need tracking of this issue on our side.

Notice Node.js does seem to reset to default correctly:

let counter = 0;
function handler() {
  console.log("Register");
  counter++;
  if (counter == 3) {
    console.log("Unregister");
    process.off('SIGINT', handler);
    // Ctrl-C after this kills the program
  }
}

process.on('SIGINT', handler);
setTimeout(() => {}, 100000);
@kevinkassimo kevinkassimo changed the title Signal defaults never restored after sig.dispose() called Signal default actions never restored after sig.dispose() called Aug 23, 2020
@stale
Copy link

stale bot commented Jan 6, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jan 6, 2021
@lucacasonato
Copy link
Member

lucacasonato commented Jan 6, 2021

@kevinkassimo Should I classify this is a bug? Is there something actionable to do here?

@stale stale bot removed the stale label Jan 6, 2021
@stale
Copy link

stale bot commented Mar 7, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Mar 7, 2021
@stale stale bot closed this as completed Mar 14, 2021
@bartlomieju bartlomieju reopened this May 9, 2021
@stale stale bot removed the stale label May 9, 2021
@bartlomieju
Copy link
Member

Tokio documentation (https://docs.rs/tokio/1.5.0/tokio/signal/unix/struct.Signal.html#caveats) explicitly states that dropping a signal handler won't restore default behavior; we need to handle that situation ourselves. Most likely we'll need to keep track of number of registered/unregistered SIGINT signal and if it goes to zero we should add default handler that exits the process.

@lucacasonato lucacasonato added bug Something isn't working correctly runtime Relates to code in the runtime crate labels Jun 8, 2021
@Cre3per
Copy link
Contributor

Cre3per commented Apr 28, 2023

Tokio uses the signal_hook crate. signal_hook has an emulate_default_handler() function that can handle more signals than a custom SIGINT handler could. It also preserves exit codes by unregistering the handler and then re-raising the signal.

nathanwhit added a commit that referenced this issue Mar 14, 2024
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working correctly runtime Relates to code in the runtime crate
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants