Handle SIGBREAK (Windows console close) #2473
Conversation
SetConsoleCtrlHandler is not needed because SIGBREAK is also sent on window close. The process is killed when the thread handling SIGBREAK returns. That's why we must handle it synchronously and wait for other threads to finish first. The Windows-defined timeout is only 5 seconds but there is no way to change that. https://docs.microsoft.com/en-us/windows/console/ctrl-c-and-ctrl-break-signals Fixes otland#1277 Fixes otland#1853
src/signals.cpp
Outdated
| #else | ||
| // this must be a blocking call as Windows calls it in a new thread and terminates | ||
| // the process when the handler returns (or after 5 seconds, whichever is earlier) | ||
| signal(SIGBREAK, dispatchSignalHandler); |
There was a problem hiding this comment.
There's one problem - dispatchSignalHandler is not signal-safe, invoking it may result in a deadlock (and loss of state).
There was a problem hiding this comment.
When we invoke it for SIGBREAK, the process is going to be terminated very soon. I don't think queuing up work from other signals would make a difference as we're shutting down anyway. Or is that not what you meant?
There was a problem hiding this comment.
As far as I understand SIGBREAK, if you use the standard API (the signal function), the handler will be invoked on an existing thread(as opposed to SetConsoleCtrlHandler which spawns a new thread that runs the handler) in an asynchronous manner. The deadlock can occur if a thread is interrupted while holding a mutex and the handler attempts to lock that mutex. Only signal-safe functions and lockfree atomic operations are allowed on mutable shared state.
There was a problem hiding this comment.
You are right about this being the expected behavior. However, in my tests, it appears that in fact a new thread is created. If the handler was running in an existing thread, it would either block entirely or it wouldn't need to "join" other threads. Yet, it does, because Windows is creating this new thread AND watching for it to finish (and then kills the rest of the process, if any).
Above is a debugger showing threads at a breakpoint set on the Signals::dispatchSignalHandler. The KernelBase.dll thread doesn't exist during normal runtime.
The handling of SIGINT looks the same, and the documentation specifically mentions:
Note When a Ctrl+C interrupt occurs, Win32 operating systems generate a new thread to handle the interrupt. This can cause a single-thread application, such as one ported from UNIX, to become multithreaded, potentially resulting in unexpected behavior.
So I believe this is a correct implementation. Also, the reason I don't exit the process with a code (e.g. ExitProcess(0)) is because after the thread returns nothing, it the "default" handler will return the special "window closed code":
The program '[9944] theforgottenserver.exe' has exited with code -1073741510 (0xc000013a).
Found online:
Exit Code 0xC000013A means that the application terminated as a result of a CTRL+C or closing command prompt window
There was a problem hiding this comment.
Windows development is harder than I thought... anyway, could you leave a mention of this fact in a comment (that dispatchSignalHandler does not need to be signal-safe on Windows)?
SetConsoleCtrlHandler is not needed because SIGBREAK is also sent
on window close. The process is killed when the thread handling
SIGBREAK returns. That's why we must handle it synchronously
and wait for other threads to finish first. The Windows-defined
timeout is only 5 seconds but there is no way to change that.
https://docs.microsoft.com/en-us/windows/console/ctrl-c-and-ctrl-break-signals
Fixes #1277
Fixes #1853