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

Multiple Signal Handler #3773

Closed
kt3k opened this issue Jan 24, 2020 · 2 comments · Fixed by #3913
Closed

Multiple Signal Handler #3773

kt3k opened this issue Jan 24, 2020 · 2 comments · Fixed by #3913

Comments

@kt3k
Copy link
Member

kt3k commented Jan 24, 2020

PR #3757 tries to add the signal handler function Deno.signal(signo). This only handles one kind of signals for each Deno.signal call.

If we add an API which handles multiple kinds of signals at once, what is the interface of that function? Where it belongs?

Option 1: Enhance Deno.signal()

Deno.signal takes multiple signals.

const sig = Deno.signal(Deno.Signal.SIGTERM, Deno.Signal.SIGINT);

for await (_ of sig) {
  console.log("SIGTERM or SIGINT received");
}

Option 2: Have a function for it in deno_std

import { signal } from "https://deno.land/std/signal/mod.ts";

const sig = signal(Deno.Signal.SIGTERM, Deno.Signal.SIGINT)

for await (_ of sig) {
  console.log("SIGTERM or SIGINT received");
}

Other: Please write in the comment

cc @axetroy

@axetroy
Copy link
Contributor

axetroy commented Jan 24, 2020

I think the first option Option 1: Enhance Deno.signal() is better

If PR #3757 land.

Then only supports one signal, but it can be extended later to support multiple signals without BREAKING CHANGE.

@kevinkassimo
Copy link
Contributor

kevinkassimo commented Jan 24, 2020

I feel it is more of a general utility: combine, multiplex and iterate 2 independent async iterators. It does not really need to be functionality built into just signal handlers.

We can implement this as a new util in std

Actually I think MuxAsyncIterator might be used here.

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 a pull request may close this issue.

3 participants