Node: listening to sigint don't exit nicely

[AndreasMadsen]

• Version: master (83c93158fb0e979dbffb4a776d237da0db8f7b08)
• Platform: MacOS
• Subsystem: process, trace_events

Run the following program, hit Ctrl+C. I would then expect got SIGINT and exit to be printed. It prints neither.

The problem that I'm really having, is that if I add --trace-events-enabled --trace-event-categories node.async_hooks to spawn then it doesn't flush the trace_events. I don't think this is a duplicate of https://github.com/nodejs/node/issues/14802, as the SIGINT handler is supposed to shut down the process nicely. But the signal handler simply doesn't execute.

const fs = require('fs');
const { spawn } = require('child_process');

const CODE = `
const http = require('http');

// Keep the process alive
const server = http.createServer();
server.listen();

process.once('SIGINT', function () {
  console.log('got SIGINT');
  server.close();
});

process.once('exit', function () {
  console.log('exit');
});
`;

const proc = spawn(process.execPath, ['-e', CODE], {
  stdio: 'inherit'
});

// relay SIGINT to process
process.once('SIGINT', function () {
  proc.kill('SIGINT');
});

[santigimeno]

From what I can tell it's working as expected. When pressing Ctrl+C in the terminal, a SIGINT signal is sent to the foreground process group, in this case the parent and child process, so both receive the signal. So the problem is that the child process is receiving two consecutive SIGINT and only handling one, so when the 2nd arrives the process exists without being able to write the logs.

[davisjam]

@santigimeno's explanation seems sound.

Wikipedia link to the behavior of Ctrl+C being delivered to all processes in the foreground process group.

If you comment out the portion of your sample titled "relay SIGINT to process" then on Ctrl+C the 'got SIGINT' and 'exit' messages are emitted.

I think this issue can be closed -- working as designed, signals are just tricky.

[AndreasMadsen]

@davisjam I agree that the child process gets two SIGINTs, that is the test. The problem is that no text is written and the log files aren't flushed correctly. SIGINT is supposed to be a soft kill, so whether it gets one or two SIGINTs, it should flush stdout and any logs. It doesn't!

[santigimeno]

@AndreasMadsen if you specifically don't handle a SIGINT signal there's no way you can guarantee any actions whether flushing stdout or any other are executed. IIUIC that's the nature of termination signals. So, in your sample code, your child process should be prepared to receive 2 consecutive SIGINT signals (not once) and handle them as you prefer.

[davisjam]

See man 7 signal. SIGINT is fatal if uncaught. Soft means it is catchable, but the default behavior is fatal and then all bets about buffered I/O are off.

[AndreasMadsen]

We have a single handler in node.cc: https://github.com/nodejs/node/blob/master/src/node.cc#L4115

[davisjam]

@AndreasMadsen I don't understand your point.

[santigimeno]

But that handler just cleans up some stuff and raises the signal.

[davisjam]

But that handler just cleans up some stuff and raises the signal.

Right. And if your application has no signal handler, SIGINT is fatal.

[AndreasMadsen]

@davisjam When pointing to the manual you are speaking about OS defaults. We have a single handler in node.js, so we are outside the defaults.

I don't want to discuss what the OS does by default. I want to point out that Node.js not printing 'got SIGINT' is ridiculous, whether it gets one or two SIGINTs.

[AndreasMadsen]

@santigimeno Exactly, as it should. It should properly also do a fflush(stdout); fflush(stderr); and the #14802 could be responsible for StopTracingAgent not doing what it should, but if I recall correctly SignalExit isn't invoked twice.

[davisjam]

Correct me if I'm wrong, but it looks like the Node handler concludes with a call to raise, re-delivering the signal to the application.

I want to point out that Node.js not printing 'got SIGINT' is ridiculous, whether it gets one or two SIGINTs.

Have you tried in other languages? I'm fairly certain you would see the same behavior in a comparable C program.

fflush(stdout); fflush(stderr);

This can be quite expensive. If your application wants to handle SIGINT it can set a handler.

[santigimeno]

It should properly also do a fflush(stdout); fflush(stderr)

The problem with that, and I think that's pointed out in https://github.com/nodejs/node/issues/14802, is that flushing a stream is not async-signal-safe

[santigimeno]

Also, even if you could flush the stream safely, there's no way to be sure your code has really executed the console.log() statement before receiving the second signal, so that solution would not work either.

[davisjam]

flushing a stream is not async-signal-safe

Right, cf. [Linux man page]http://man7.org/linux/man-pages/man7/signal-safety.7.html) describing which functions are async-signal-safe.

[davisjam]

@AndreasMadsen Does this all make sense? It seems to me that this issue can be closed with "Linux is working as designed".

[AndreasMadsen]

@davisjam It makes sense, I still disagree. Node.js is not Linux and it is not C. These comparisons don't make any sense. The only valid comment I have seen here is:

there's no way to be sure your code has really executed the console.log() statement before receiving the second signal, so that solution would not work either.

So I need to look into whether the console.log() statement was executed.

[davisjam]

Node.js is not Linux and it is not C. These comparisons don't make any sense.

If a Node.js application is running on Linux and signals are delivered to it, then the Node.js application will respond according to the Linux signal mechanism's semantics. Those semantics include:

1. An unhandled SIGINT is fatal, and
2. You can't flush streams from a signal handler.

Given these semantics, what are you proposing be changed about Node's behavior?

[mattflix]

A lot of arguing back and forth in this issue.

But... it seems to me that everyone missed (or at least the OP did) the fact the OP's code registers for SIGINT using .once() (as opposed to.on()), which explains the behavior the OP is seeing.

That is, since the OP's parent process is coded to explicitly send the child process a _second_ SIGINT, the second signal is given default processing by Node within the child process (i.e., immediate termination) thanks to .once() having removed the override handler upon receipt of the first signal.

If the original poster were to alter the child process to use .on(), or alter the parent process _not_ explicitly send a second SIGINT, then I believe they will see the behavior they expect (i.e., child process allowed to gracefully terminate).

Either of these changes indeed do make the parent/child work as expected on Mac OS (and I assume Linux). But unfortunately, not on Windows, where, seemingly, there is some bug in Node's signal emulation that causes the second SIGINT to not be delivered to the child's override (instead terminating it) even if the child maintains the handler using .on().

[bzoz]

There is no bug here, Windows just does not support sending signals.

[mattflix]

One man's lack-of-functionality is another man's bug. ;)

There are plenty of IPC mechanisms available between Windows processes that Node.js could employ to better-emulate *nix-style signals, at least when the processes involved are simply instances of the same version of node.exe.

[bzoz]

You never can be sure what Node versions will end up talking to each other: like https://github.com/nodejs/node/issues/21671

If you can come up with a method to emulate signals, go ahead. PRs are always welcomed.

[Trott]

@AndreasMadsen In your opinion, is there still something to look into here? It seems like things were left a bit ambiguous, and it's been quite a while. Not feeling strongly about this issue one way or the other, but will certainly close it to tidy things up if there's nothing more to do/say here in your opinion.

[AndreasMadsen]

@Trott I see nothing ambiguous about this. I made a very clear test case. I think the dialog has been really unproductive because a few individuals are more focused on explaining how Linux handles signals by default, rather than openly discussing what would be useful for the Node.js runtime in terms of debugging.

[sam-github]

@AndreasMadsen Doesn't your testcase have a bug, as pointed out in https://github.com/nodejs/node/issues/18600#issuecomment-419685146?

[Trott]

@AndreasMadsen Doesn't your testcase have a bug, as pointed out in #18600 (comment)?

@sam-github Doesn't that comment basically say that the problem with the test case is that it needs to handle the possibility of getting more than one SIGINT? @AndreasMadsen already indicated that the fact that the subprocess gets two SIGINTs is, in their view, the bug.

[Trott]

openly discussing what would be useful for the Node.js runtime in terms of debugging.

OK, so the points of discussion would seem to be:

• Would the proposed behavior change be an improvement over the current behavior or not?
• Is the proposed behavior change feasible? In other words, can it be done without a potential big performance cost or introducing other bugs?

I don't know the answers myself. and I'm not suggesting anyone in particular should be obligated to do the work to figure out the answer to the second bullet point. But it seemed like a summary would be helpful (especially if my summary is wrong, because then someone can correct me), so there you go.

[sam-github]

Would the proposed behavior change

What is the proposed behaviour change?

[Trott]

What is the proposed behaviour change?

I think it's that the stdout and files should be flushed and the console.log() in the handler should be guaranteed to produce output. @AndreasMadsen will need to confirm that I'm not misunderstanding, though.