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 Handlers #2339

Closed
kt3k opened this issue May 12, 2019 · 20 comments · Fixed by #3757
Closed

Signal Handlers #2339

kt3k opened this issue May 12, 2019 · 20 comments · Fixed by #3757
Labels
feat new feature (which has been agreed to/accepted)

Comments

@kt3k
Copy link
Member

kt3k commented May 12, 2019

Something equivalent of the followings:

node.js

process.on('SIGTERM', () => {/* do something */}));

python

def handler(signum, frame):
  # do something

signal.signal(signal.SIGTERM, handler)

ruby

Signal.trap("TERM") {
  # do something
  exit
}

golang

sigc := make(chan os.Signal, 1)
signal.Notify(sigc, syscall.SIGTERM)
go func() {
    s := <-sigc
    // do somethig
}()

rust (using tokio signal)

let ctrl_c = tokio_signal::ctrl_c().flatten_stream();

let prog = ctrl_c.for_each(|()| {
    // do something
    Ok(())
});

tokio::run(prog.map_err(|err| panic!("{}", err)));
@kitsonk
Copy link
Contributor

kitsonk commented May 13, 2019

Seems like a good idea to me. The challenge will be finding the right API.

The Node.js model is basically an event emitter. We don't have a whole lot of them in Deno at the moment. It would be more "Deno" like to have an async iterator, which is closer to Rust/tokio signal. Something like this:

for await (const signal of Deno.signals.SIGTERM) {
  /* ... */
}

@kevinkassimo
Copy link
Contributor

kevinkassimo commented May 13, 2019

Async iterator makes sense. I think I have some basic idea of implementation. Will see if I could do this in the coming week.

On the other hand, I think I might need to introduce something like an optional_ops that, unlike normal pending_ops does not prevent Deno from exiting (I have a prototype for this)

@zekth
Copy link
Contributor

zekth commented May 13, 2019

It may be revelant and user friendly to wrap the async iterator in a function like:

async Deno.signals.Action(signal: Deno.signal, action: () => void)

By this way in its program the developer has just to write:

Deno.signals.Action(Deno.signals.SIGTERM,() => { console.error('SIGTERM'); process.exit(1);})
Deno.signals.Action(Deno.signals.SIGKILL,() => { console.error('SIGKILL'); process.exit(1);})

Because not everyone is familiar with async iterator.

@kitsonk
Copy link
Contributor

kitsonk commented May 13, 2019

Because not everyone is familiar with async iterator.

There are if they use our file interfaces, or our web server interfaces. We don't have convenience wrappers for those. Not promoting use of the language doesn't benefit anyone.

If people aren't familiar with async iterators, that is what code examples and documentation are for.

@zekth
Copy link
Contributor

zekth commented May 13, 2019

Not promoting use of the language doesn't benefit anyone

This is not because you provide an additionnal wrapper that you do not promote the use of the language. Async iterators are nice but in this case I'd rather write :

Deno.signals.Action(Deno.signals.SIGTERM, foo);
Deno.signals.Action(Deno.signals.SIGKILL, foo);

instead of:

for await (const signal of Deno.signals.SIGTERM) {
  foo();
}
for await (const signal of Deno.signals.SIGKILL) {
  foo();
}

@zekth
Copy link
Contributor

zekth commented May 13, 2019

About the wrapper @bartlomieju just told me something interesting to not use this:

I don't think if coercing such thing to look like in sync code is a good idea

@keroxp
Copy link
Contributor

keroxp commented May 15, 2019

I agree with @zekth . for await is not suitable for async event handler because it generally degrades concurrency of handler. It should only be used for serial execution of promises. We're discussing to remove serve() from HTTP handler interface from std: denoland/std#386

About signal, There may not be good point to handle signals in serial. Each signal must be handled immediately even if they are same.

@ry
Copy link
Member

ry commented Sep 13, 2019

Added to 1.0 blockers

@ry ry changed the title Feature Request: Signal Handlers Signal Handlers Sep 13, 2019
@ry ry mentioned this issue Sep 13, 2019
43 tasks
@ry ry added the feat new feature (which has been agreed to/accepted) label Sep 13, 2019
@kevinkassimo
Copy link
Contributor

Also see #2735 as reference for future implementation attempts

@sholladay
Copy link

sholladay commented Sep 23, 2019

It is possible to construct an object that is both a thenable and an iterable, meaning you can use it as a Promise or like a Generator.

const sigterm = Deno.signals.sigterm();
// A. Handle only the next SIGTERM event
sigterm.then(handler);
// B. Handle all future SIGTERM events
for await (const event of sigterm) {
  handler(event);
}

The cool thing about doing it this way is that in simple cases you can just use A in the example above, which is a very concise one-liner.

I don't think it is as important to make B compact, because the kinds of handlers that need to run on every event tend to be more complex, anyway.

For an example of implementing a thenable generator, see sindresorhus/emittery.

@hazae41
Copy link
Contributor

hazae41 commented Jan 16, 2020

Emittery provides on(), once(), and an async iterator.

It seems good for signals to have all three.

@phil294
Copy link

phil294 commented Mar 12, 2020

@hazae41 It seems however, that your pull request only implements the async iteratior solution, doesnt it? What is the motivation behind not adding the event listeners?

If you need to listen for two different signals repeatedly, your code might look something like this:

(() => {
  for await (const _ of Deno.signal(Deno.Signal.SIGINT)) {
    console.log("interrupt signal")
  }
})()
(() => {
  for await (const _ of Deno.signal(Deno.Signal.SIGQUIT)) {
    console.log("quit signal")
  }
})()

Specially in single file scripts, I think classical event handlers would make more sense:

Deno.signal.on(Deno.Signal.SIGINT, () => console.log("interrupt"))
Deno.signal.on(Deno.Signal.SIGQUIT, () => console.log("quit"))

I understand that Deno tries to avoid everything about node's callback hell, but why not use event listeners for listening to events?

@kt3k
Copy link
Member Author

kt3k commented Mar 12, 2020

@phil294
I didn't add event handler interface because we prefer to keep the core as simple as possible.
You can convert async iterator to event handler like the below:

async function onSignal(signo: number, callback: () => void) {
  for await (const _ of Deno.signal(signo)) {
    callback();
  }
}

onSignal(Deno.Signal.SIGINT, () => console.log("interrupt"));
onSignal(Deno.Signal.SIGQUIT, () => console.log("quit"));

We also have std/signal standard module ( https://github.com/denoland/deno/blob/master/std/signal/mod.ts ). It might be a good idea to suggest the addition of the above to std.

@sholladay
Copy link

sholladay commented Mar 12, 2020

I still think this ...

Deno.signal(Deno.Signal.SIGINT);

... should be shortened to:

Deno.signal.sigint();

Also, regarding handling multiple signals, I would expect an API like this to be built into core (but std would be fine, too):

for await (const _ of Deno.signals(Deno.signal.sigint(), Deno.signal.sigquit())) {
    console.log('interrupt or quit signal');
}

In fact, in the above example, Deno.signals() doesn't really need to be directly related to signals at all. It's just a function that combines two or more async iterables. Really it's something that should be built into JavaScript, like Promise.all().

@kt3k
Copy link
Member Author

kt3k commented Mar 14, 2020

@sholladay
We have the following shorthand methods:

Deno.signals.alarm(); // for SIGALRM
Deno.signals.child(); // for SIGCHLD
Deno.signals.hungup(); // for SIGHUP
Deno.signals.info(); // for SIGINFO
Deno.signals.interrupt(); // for SIGINT
Deno.signals.io(); // for SIGIO
Deno.signals.pipe(); // for SIGPIPE
Deno.signals.quit(); // for SIGQUIT
Deno.signals.terminate(); // for SIGTERM
Deno.signals.userDefined1(); // for SIGUSR1
Deno.signals.userDefined2(); // for SIGURS2
Deno.signals.windowChange(); // for SIGWINCH

The names are inspired by the tokio's SignalKind enum ( https://docs.rs/tokio/0.2.13/tokio/signal/unix/struct.SignalKind.html )

We have the multiple signal handler utility in std, which works like the below:

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

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

setTimeout(() => {}, 5000) // Prevents exiting immediately

for await (const _ of sig) {
  console.log("interrupt or usr1 signal");
}

@rivy
Copy link
Contributor

rivy commented Apr 4, 2021

Windows implementation?

@phil294
Copy link

phil294 commented Nov 9, 2021

I understand that Deno tries to avoid everything about node's callback hell, but why not use event listeners for listening to events?

For anyone still following this thread, the callback based approach has now been implemented after all in #12512

@rivy
Copy link
Contributor

rivy commented Nov 9, 2021

It seems that it's still "not implemented" for Windows. 😞

C:>deno --version
deno 1.16.0 (release, x86_64-pc-windows-msvc)
v8 9.7.106.2
typescript 4.4.2

C:>deno eval -T --unstable "const listener = () => { console.log('SIGINT seen') }; Deno.addSignalListener('SIGINT', listener); const delay = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); await delay(10000);"
Check a generated module
error: Uncaught Error: not implemented
    at Object.opSync (deno:core/01_core.js:142:12)
    at bindSignal (deno:runtime/js/40_signals.js:12:17)
    at Object.addSignalListener (deno:runtime/js/40_signals.js:50:21)
    at file:///C:/Users/Roy/OneDrive/Projects/deno/dxx/repo.GH/$deno$eval.ts:1:61

@phil294
Copy link

phil294 commented Nov 9, 2021

@rivy there are no signals in Windows, only APIs like postMessage(). Long live POSIX. What are you trying to do?

@rivy
Copy link
Contributor

rivy commented Nov 9, 2021

@rivy there are no signals in Windows, only APIs like postMessage(). Long live POSIX. What are you trying to do?

Catch signals (or emulated signals) portably (similar to NodeJS) for clean exits from scripts; eg, from #9995...

// ...works in NodeJS...

[`SIGBREAK`, `SIGINT`, `SIGUSR1`, `SIGUSR2`, `uncaughtException`, `SIGTERM`].forEach((eventType) => {
	process.on(eventType, catchSignal.bind(null, eventType));
});
[`exit`].forEach((eventType) => {
	process.on(eventType, catchExit.bind(null, eventType));
});

function catchSignal(eventType) {
	console.log('Caught signal...', { eventType });
}
function catchExit(eventType) {
	console.log('Now exiting...', { eventType });
}

setTimeout(() => {
	console.log('The service is now finished.');
}, 2000);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat new feature (which has been agreed to/accepted)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants