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

feat(runtime): add Deno.addSignalListener API #12512

Merged
merged 5 commits into from
Oct 26, 2021

Conversation

kt3k
Copy link
Member

@kt3k kt3k commented Oct 21, 2021

This PR implements Deno.addSignalListener and Deno.removeSignalListener APIs as disucessed in #11158

The basic usage:

const listener = () => {
  console.log("SIGTERM!")
};
// Starts listening to SIGTERM
Deno.addSignalListener("SIGTERM", listener);
...
// Stops listening to SIGTERM
Deno.removeSignalListener("SIGTERM", listener);

closes #11158

@bartlomieju bartlomieju added this to the 1.16.0 milestone Oct 21, 2021
@ry ry requested a review from piscisaureus October 21, 2021 11:55
}
}
} catch (e) {
if (e instanceof errors.BadResource) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does BadResource get thrown?
I think it could only happen if there is an error in the listener callback, in which case we should not be handling it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. This handling was unnecessary. Updated.

Copy link
Member

@piscisaureus piscisaureus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM too, I like this API better; it will be useful for std/node

@bartlomieju
Copy link
Member

BTW, should we also stabilize it in 1.16?

@bartlomieju
Copy link
Member

@kt3k please land ASAP as this PR will unblock polyfilling more process.on() events in std/node.

@piscisaureus
Copy link
Member

BTW, should we also stabilize it in 1.16?

Let's get some experience with it before we decide.
I have some concern about what happens when you attach an async function as a listener.
If a signal is fired multiple times (e.g. press ^C twice) you might end up with the same cleanup process running at the same time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: Callback based Signal API
3 participants