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

SIGINT and loops #9050

Closed
Revision18 opened this issue Oct 12, 2016 · 8 comments
Closed

SIGINT and loops #9050

Revision18 opened this issue Oct 12, 2016 · 8 comments
Labels
doc Issues and PRs related to the documentations. process Issues and PRs related to the process subsystem.

Comments

@Revision18
Copy link

Version: v4.2.6
Platform: x86_64 GNU/Linux

I don't think this is a bug but some kind of advice would be nice in the documentation of process.on() in the future. Please, consider it.

If your script is structured following the design of a C program, chances are you may have a main loop similar to this:

while (true)
{
    // Your program
}

If you suddenly need to do something on SIGINT, you may do this:

process.on("SIGINT", function () {
    // remove incomplete output files because user interrupted the script with CTRL+C
    process.exit();
});

while (true)
{
    // Your program
}

This results in two issues:
1- As noted in documentation, node does not exit when you assign a listener to SIGINT, you must call process.exit() if you want to exit, but...
2- when you are inside the while loop, the event isn't triggered. Resulting in the script trapped in the loop and with no nice way of stopping. User must kill the process: close terminal, use Gnome System Monitor, send a SIGKILL (to node process) from pkill or similar commands.

I solved it and achieved the behavior I need by changing the while loop to this:

var interval = setInterval (function () {
    // Your program
}, 0);

Similar to what you may do with javascript in client side code in a web browser to simulate a main loop.

This way, when user hits CTRL+C the event handler executes as expected.

This is not a bug, just how javascript code is executed, but if you don't know at the moment what happens when you register a listener for SIGINT you may expect that it works even while your program is inside a loop.

It took me a while to realize why CTRL+C wasn't working so I'm leaving this here.

@bnoordhuis bnoordhuis added doc Issues and PRs related to the documentations. process Issues and PRs related to the process subsystem. labels Oct 12, 2016
@cjihrig
Copy link
Contributor

cjihrig commented Oct 12, 2016

I'm not sure what the best way to document this would be (or even if we need to). Maybe open a PR and see how it goes.

@gibfahn
Copy link
Member

gibfahn commented Oct 12, 2016

At the very least this issue (with included fix) should show up in Google if someone searches "nodejs sigint loops", which is some form of documentation... (although by all means open a PR as well if you think of a good way/place to document it).

@Revision18
Copy link
Author

Revision18 commented Oct 12, 2016

OK. Can I include a link to this issue? I don't remember to see links to github issues in the docs before.

I was thinking in some kind of additional warning in the same paragraph were the documentation states that if you register a listener for SIGINT then node won't exit by itself anymore. But I don't sure how to word it as English is not my natural language.

The current warning alone, as It happened with me, isn't enough to prevent all people from expecting that a CTRL+C will stop execution of the script whatever it is doing at the moment. When no listener is registered that is exactly what happened so you may expect that the same occurs when you finally register a listener.

I have to add more, something that I noticed today:

If you use child_process.execSync("shell command") in combination with SIGINT you must be aware of the following. SIGINT will terminate child processes, and if you aren't using any listeners for it, node will terminate too. The problem, again, is if you register a listener for SIGINT, then something very inconvenient happens. The child process ends immediately at the press of CTRL+C (if computer isn't lagged), because the signal propagates, but your script continues until complete the current iteration of the loop. In this case, setInterval sill may produce corrupted output, depending of what you are doing, as the current iteration will run fully and your SIGINT handler will not run until it finishes.

setInterval isn't enough in this case, you need to have two processes, the one reacting to SIGINT, and your true program as a child process. This is only an idea, I didn't tried it yet, but in my situation is what I will have to do to achieve the result I'm pursuing.

The following is untested:

Wrapper script:

// Avoid script termination without using loops or intervals
process.stdin.resume();

var child = null;

process.on('SIGINT', function () {
    // Your logic to determine what needs to be done like remove potentially  incomplete temp files
    // In my case, I'm using another empty file to communicate the wrapper script that the inner script
    // finished the output and the file needs not to be removed.
    var stats = null;
    try {
        stats = fs.statSync("temp.ok");
    }
    catch (error) {
    }

    if (stats === null) {
        // Remove potentially incomplete output file
    }

    if (child !== null)
        child.kill('SIGINT');

    process.exit();
});

// Needs to be async or you are in the same situation than before and the wrapper is useless
child = child_process.exec("nodejs your_script.js");

Your true script (the while loop is back, and It's harmless this time):

while (true)
{
    child_process.execSync("whatever");

    // If this line is reached, create the empty file to tell the wrapper that the output is complete
    // and needs not to be removed. If user press CTRL+C before this line, just let the wrapper
    // think the output may be incomplete and remove the file.
    var f = fs.openSync("temp.ok", "wx");  // let it throw, the file isn't supposed to be there
    fs.closeSync(f);
}

All this to achieve a similar behavior to a C program, where CTRL+C interrupts the process whatever it is doing at the time. The only way to achieve that in nodejs is to let node handle the CTRL+C, as the behavior changes when you register a listener. So I expect this to be the solution, use a wrapper to register the listener and let the true script, the one that does the actual job, without any listener so nodejs will terminate immediately when It receives the SIGINT (or that is what I expect/want to occur).

People that have present at all times how the engine runs javascript code may not need this warning but if this took myself by surprise then another person may be found off-guard in the future.

@mk-pmb
Copy link

mk-pmb commented Oct 22, 2016

As noted in documentation, node does not exit when you assign a listener to SIGINT, you must call process.exit() if you want to exit,

Not on my node.

$ node-versions
Node.js v6.8.1, npm v3.10.8, Ubuntu 14.04.5 LTS trusty
$ time nodejs -e 'process.on("SIGINT", console.log.bind(console, "int!"));
  setTimeout(console.log.bind(console, "timeout!"), 5000);'
timeout!

real    0m5.103s

My node terminated on its own after about 5 seconds.

setInterval

It might be preferable to make a function that re-schedules itself when it is done with one iteration. (Reasons)

If your script is structured following the design of a C program,

… then you're missing out on a lot of cool language features, or may even be unaware of some of JavaScript's pitfalls. (Details)

The problem, again, is if you register a listener for SIGINT, then something very inconvenient happens.

I think you're confused about the reasons here.

but in my situation is what I will have to do to achieve the result I'm pursuing.

Nah, there's still hope! For example, you could try to write JavaScript the way it was intended: async.

The only way to achieve that in nodejs is to let node handle the CTRL+C,

This claim is just wrong. Don't worry, most people just start writing JS without bothering to learn the language, but it's really not a Node bug if you organize your JS program like a C program.

Don't busy-wait for the child process to finish. Instead, set up event handlers for what shall be done when the child process finishes. Meanwhile node can do other things, like listening for SIGINT and call your SIGINT handler. Your handler can then send SIGINT to the child process, and set a flag for your self-re-scheduling function so it knows that on its next turn, it shall begin clean shutdown. (In case it doesn't make sense to you right now, try watching the pitfalls talks first.) If you have more questions about program structure, feel free to ask in the #node.js IRC channel on Freenode.

@mk-pmb
Copy link

mk-pmb commented Oct 22, 2016

Now for my real reason I'm here. I understand why node won't react to SIGINT while I have a repeating SIGINT handler registered. In that case I expect it to queue up all the events and deliver them asap.

In my program I only want to handle the first SIGINT, because if a second one arrives, I assume my program wasn't able to shut down quickly enough, and therefor on 2nd SIGINT I assume malfunction and want to let node deal with that emergency.

It works fine if the malfunction is just that I forgot a clearTimeout():

$ node-versions
Node.js v6.8.1, npm v3.10.8, Ubuntu 14.04.5 LTS trusty
$ time nodejs -e 'process.once("SIGINT", console.log.bind(console, "int!"));
  setTimeout(console.log.bind(console, "timeout!"), 5000);'
^Cint!
^C

real    0m0.754s

My expectation what happened: On first SIGINT, node queues the event for my handler, releases that handler. On the next event loop iteration, my handler runs and prints int. 2nd SIGINT arrives, no custom handler, node quits.

What I don't understand is the case where the malfunction is some sync badness¹ like busy waiting:
(¹ More realistic scenario: require.resolve() trying to read from a now-offline network file system due to some left-over debug symlink.)

$ time nodejs -e 'process.once("SIGINT", console.log.bind(console, "int!"));
  const blockUntil = Date.now() + 5e3; while (Date.now() < blockUntil) {}'
^C^C^C^C^C
real    0m5.097s

Almost looks like a run of the event loop was required in order for node to reset its C-language SIGINT handler after the first event is put on todo. Can we improve that user experience aspect?

@bnoordhuis
Copy link
Member

Can I include a link to this issue?

I don't think we do that as a rule. For one, it would be useless to people working in offline mode.

Almost looks like a run of the event loop was required in order for node to reset its C-language SIGINT handler after the first event is put on todo.

That's exactly what happens. The signal is caught by libuv, not node.js. It's not until the event is delivered to node.js, that it can reset the handler.

It should be possible to add a one-shot mode to libuv. I've filed libuv/libuv#1104 but I won't be working on it.

@mk-pmb
Copy link

mk-pmb commented Oct 23, 2016

Thanks!

@Revision18
Copy link
Author

Revision18 commented Oct 24, 2016

I knew the async vs sync will come out sooner or later. I know that async is how JavaScript is meant to be used. For the rest of the discussion, please, let's assume that I have my good reasons to prefer sync versions of everything for this specific script (the reason is not that I'm porting code from C, I'm not, actually).

I admit I didn't studied JavaScript formally, I'm just running into things and learning while using it (as most people?). I find it more convenient and easy to use than Python, for example, and far more powerful than shell scripts, to automate things. I thank you for the material you linked, I will consume it.

Not on my node.

OK. Something changed between my node version and yours, maybe? Because docs say it should not exit.

Almost looks like a run of the event loop was required in order for node to reset its C-language SIGINT handler after the first event is put on todo.

And for me, this makes me unable to register the SIGINT handler in the same process that does the actual job. As I don't want to go async, I need the node process to stop as soon as the signal arrives, because the child process will do that, and an inconsistency here makes my script produce an incomplete or corrupt output, depending on the case, as it uses OS commands to do its job. Yes, I can write some logic to detect that the process terminated early and the incomplete output needs to be removed, but by having node handling the SIGINT this is not necessary. So for now the weird solution I proposed of having a parent process handling the SIGINT is working for me (I know is very anti JavaScript, and may be not RAM friendly because now you have two node instances), this way when the signal arrives, the script doesn't need to finish a full iteration of any timeout/interval it may be in (at least for what I observed, when you don't register for listen to the signal, node doesn't need to wait of a full iteration). The other solution would be to go async and forget of all this. When I finally upload this project may you then understand my reasons to not want this.

I think there is no reason to keep this issue opened. I will close it in a moment. Thank you people for your participation.

santigimeno added a commit to santigimeno/node that referenced this issue Jun 23, 2017
`one-shot` signal handlers reset the handler once the signal is received
and not when the signal notification reaches the main loop. This commit
adds support for this functionality when *only* there are `once`
listeners on a specific signal.

Refs: nodejs#9050
Refs: libuv/libuv#1104
Refs: libuv/libuv#1106
santigimeno added a commit to santigimeno/node that referenced this issue Nov 15, 2017
`one-shot` signal handlers reset the handler once the signal is received
and not when the signal notification reaches the main loop. This commit
adds support for this functionality when *only* there are `once`
listeners on a specific signal.

Refs: nodejs#9050
Refs: libuv/libuv#1104
Refs: libuv/libuv#1106
santigimeno added a commit to santigimeno/node that referenced this issue Dec 2, 2017
`one-shot` signal handlers reset the handler once the signal is received
and not when the signal notification reaches the main loop. This commit
adds support for this functionality when *only* there are `once`
listeners on a specific signal.

Refs: nodejs#9050
Refs: libuv/libuv#1104
Refs: libuv/libuv#1106
stefan-lacatus added a commit to stefan-lacatus/ThingCLI that referenced this issue Dec 30, 2023
…emove the custom SIGINT handler.

Because emitting code is behaving similarly to an loop, handling SIGINT causes the handler to fire too late.
See nodejs/node#9050
stefan-lacatus added a commit to stefan-lacatus/ThingCLI that referenced this issue Apr 16, 2024
…emove the custom SIGINT handler.

Because emitting code is behaving similarly to an loop, handling SIGINT causes the handler to fire too late.
See nodejs/node#9050
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. process Issues and PRs related to the process subsystem.
Projects
None yet
Development

No branches or pull requests

5 participants