Handling of CTRL_CLOSE_EVENT/SIGHUP on Windows causes races and deadlocks

[mika-fischer]

• Version: v7.2.1
• Platform: Windows 8/10 64-bit
• Subsystem: process

On Windows, when a console window is closed, a CTRL_CLOSE_EVENT is sent to all processes running in that console. Applications can register handlers for this event, and all registered handlers for all processes in the console are run serially. After the handlers for a process finish, the process is automatically terminated by windows. Further, there's a timeout of five seconds. If the handler is not finished after five seconds, Windows also terminates the process. It's also worth mentioning that Windows creates a new thread in the target process and runs the handler functions in this new thread.

For further details about this see the documentation for SetConsoleCtrlHandler and HandlerRoutine.

libuv maps this event to SIGHUP, so when registering an event listener for SIGHUP on Windows, libuv sets up a handler for CTRL_CLOSE_EVENT. This handler emits a SIGHUP event, so that the registered listener is called. And after that, it calls Sleep(INFINITE), which just causes the current thread to block forever.

While this approach is understandable - as not sleeping would just kill the process - the current implementation causes several serious issues:

I have prepared a repository with scripts demonstrating the issues here: https://github.com/mika-fischer/nodejs-sighup-windows

First of all, once a SIGHUP handler is registered and a CTRL_CLOSE_EVENT received, it's no longer possible to terminate node properly. Probably the running thread somehow keeps the process alive. See on-SIGHUP-keeps-node-alive.js.

This is not completely catastrophic due to the fact that Windows will kill the process after five seconds. However this means that the SIGHUP handlers for all other processes in the console get delayed by 5 seconds, since they are called serially. Using nodejs cluster mode with 12 workers, it would already take a minute to shut down the system...

OK, so we can work around this by terminating ourselves after receiving SIGHUP, by calling process.kill(process.pid), which works for killing us. But as a side effect, it will cause the next CTRL_CLOSE_EVENT handler in another process to be called. And if this process is already shutting down, this can lead to further race conditions and deadlocks:

Once a SIGHUP handler is registered, it's no longer safe to exit the application normally. Even after the event loop is drained and node is shutting down, a CTRL_CLOSE_EVENT could arrive. Since the SIGHUP handler is still registered, a SIGHUP event will be emitted (but never received by anyone, since node has already shut down) and the Sleep(INFINITY) will kick in, causing the process to hang.
See forktest.js.

OK, so we just need to remove our SIGHUP handler, right? Unfortunately, this leads to the next problem:

Once a SIGHUP handler is registered, it's no longer safe to remove it. For some reason, after receiving a CTRL_CLOSE_EVENT, trying to unregister the handler completely blocks execution of nodejs. This is the most serious issue here. See removeAllListeners-SIGHUP-after-SIGHUP-received-blocks-event-loop.js.

Taken together, this makes it completely impossible to properly shut down nodejs on Windows in response to SIGHUP/CTRL_CLOSE_EVENT. The only option is to always forcefully kill the node process once a handler for SIGHUP is set up and never trying to remove the handler.

I think a nicer behaviour would be:

• Always allow removal of the SIGHUP event listener, even when a CTRL_CLOSE_EVENT was received.
• If node terminates normally and the Sleep(INFINITY) is all that's keeping it running, kill the thread and terminate properly.

[sam-github]

/to @nodejs/platform-windows @saghul
/cc @piscisaureus

[piscisaureus]

OK, so we can work around this by terminating ourselves after receiving SIGHUP, by calling process.kill(process.pid), which works for killing us. But as a side effect, it will cause the next CTRL_CLOSE_EVENT handler in another process to be called.

Does process.exit() not work to terminate the process?

For some reason, after receiving a CTRL_CLOSE_EVENT, trying to unregister the handler completely blocks execution of nodejs.

This is probably because SetConsoleCtrlHandler() (called here) blocks if there is a handler currently running. It could probably be worked around by just not calling that function after we've received a CTRL_CLOSE_EVENT.

If node terminates normally and the Sleep(INFINITY) is all that's keeping it running, kill the thread and terminate properly.

I think the easiest fix is, in the CTRL_CLOSE_EVENT handler, to wait for the main thread, rather than to sleep indefinitely. So it becomes something like:

HANDLE main_loop_thread_handle = ... ; // hot sure how to get it, but it's possible.
WaitForSingleObject(main_thread_handle, INFINITE);

[sam-github]

@digitalinfinity Not sure if you saw this, but some Windows talent could go a long way here!

[mika-fischer]

@piscisaureus No, process.exit() does not help at all. It goes through all the exit logic and while node is shutting down, a CTRL_CLOSE_EVENT can arrive, which causes the hang...

I did some debugging using the pdb file for node and can now provide stack traces for the two cases of the hang.

For the hang that happens after receiving CTRL_CLOSE_EVENT when node shuts down normally (i.e. on-SIGHUP-keeps-node-alive.js), it seems to hang in node, not libuv, specifically in SigintWatchdogHelper::Stop(), which indeed does call SetConsoleCtrlHandler to unregister node's own handler function. It's probable that this call does indeed block while a handler is running...

The hang caused by unregistering the SIGHUP listener after receiving a CTRL_CLOSE_EVENT (i.e. removeAllListeners-SIGHUP-after-SIGHUP-received-blocks-event-loop.js), hangs in libuv, specifically uv__signal_unregister, which also calls SetConsoleCtrlHandler to unregister libuv's handler function.

I also played around with SetConsoleCtrlHandler in C++ and it seems unlikely that the Sleep(INFINITY) does keep the process from closing, so I think that can be left as is. Just the calls to SetConsoleCtrlHandler to unregister the handler need to be either conditional on whether CTRL_CLOSE_EVENT was received, which might be difficult (since both node and libuv have their own handlers installed). Or maybe they could be omitted altogether in all cases, by having the node and libuv handler functions always return false, which is basically the same as not having the handler at all.

[bzoz]

If calling SetConsoleCtrlHandler to unregister is the problem, we could keep it registered and use a flag to mark it as "unregistered".

In any case, using process.once('SIGHUP', ...) hangs node, and is used in lib/_debugger.js

[mika-fischer]

@bzoz Yes, I think this would fix both issues.

[bzoz]

I've added flag to libuv and node_watchdog to mark handlers as "removed" instead of removing them with SetConsoleCtrlHadler (see: master...janeasystems:bartek-node-fix-10165). It does seem to fix both issues - on-SIGHUP-keeps-node-alive.js exits and removeAllListeners-SIGHUP-after-SIGHUP-received-blocks-event-loop.js also works. If you want go ahead and test.

I still want to test this change, if everything is OK I'll probably open PRs on Monday.

[mika-fischer]

@bzoz Great, thanks a lot! It looks good to me, but I'll try to compile node next week and test it as well.

[mika-fischer]

I've tested it and the fix works for me, as well.