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 Handling for windows #13

Open
tegefaulkes opened this issue Jul 14, 2022 · 2 comments
Open

Signal Handling for windows #13

tegefaulkes opened this issue Jul 14, 2022 · 2 comments
Labels
development Standard development r&d:polykey:supporting activity Supporting core activity

Comments

@tegefaulkes
Copy link
Contributor

Specification

Windows handles it's signals differently. According to https://nodejs.org/docs/latest-v16.x/api/process.html#processkillpid-signal.

"On Windows, where POSIX signals do not exist, the signal argument will be ignored, and the process will be killed forcefully and abruptly (similar to 'SIGKILL'). See Signal Events for more details.".

However acording to the node signals https://nodejs.org/docs/latest-v16.x/api/process.html#signal-events

  • SIGHUP is generated on Windows when the console window is closed, and on other platforms under various similar conditions. See signal(7). It can have a listener installed, however Node.js will be unconditionally terminated by Windows about 10 seconds later. On non-Windows platforms, the default behavior of SIGHUP is to terminate Node.js, but once a listener has been installed its default behavior will be removed.
  • SIGBREAK is delivered on Windows when Ctrl+Break is pressed. On non-Windows platforms, it can be listened on, but there is no way to send or generate it.
  • SIGINT from the terminal is supported on all platforms, and can usually be generated with Ctrl+C (though this may be configurable). It is not generated when terminal raw mode is enabled and Ctrl+C is used.'SIGINT' from the terminal is supported on all platforms, and can usually be generated with Ctrl+C (though this may be configurable). It is not generated when terminal raw mode is enabled and Ctrl+C is used.
  • SIGTERM is not supported on Windows, it can be listened on.

So we need to make sure that all of the signals that windows can produce are supported by Polykey. Then we need to create new tests testing that this works.

Additional context

Tasks

  1. manually test and confirm what signals are produced when
    • polykey is killed used ctrl + C in the terminal running it.
    • killed by closing the terminal running it.
    • killed by using task manager.
  2. Add any missing handlers for the signals produced by windows.
  3. add tests confirming the new handlers work.
@CMCDragonkai
Copy link
Member

All signals should be tested on all platforms.

If SIGTERM doesn't exist on Windows, the equivalent signal should be used for windows termination.

The main problem is that our tests assume SIGTERM is usable. But obviously on windows this is not the case, where SIGTERM is ignored.

2 things are important here:

Sending SIGINT, SIGTERM, and SIGKILL will cause the unconditional termination of the target process, and afterwards, subprocess will report that the process was terminated by signal.

I don't know how node.js maps console events to C signals, but in Microsoft's C runtime, SIGINT is mapped to CTRL_C_EVENT (cancel via Ctrl+C) and all others are mapped to SIGBREAK, which includes CTRL_BREAK_EVENT (Ctrl+Break) and CTRL_CLOSE_EVENT (the console session is closing). The C runtime only supports SIGTERM internally within the current process via raise(SIGTERM), so it's really pointless
https://stackoverflow.com/questions/61655836/sigterm-in-node-js-on-windows

So does this mean that if we switch to using SIGINT in our tests, that on Windows, it would still cause immediate shutdown and not graceful shutdown?

The solution might be something like this:

  1. Make the PK listen on SIGINT, SIGBREAK, SIGHUP, SIGTERM, and direct all of these signals to graceful shutdown.
  2. Change the tests to send SIGINT instead of SIGTERM. However this requires confirmation that SIGINT can actually be handled on windows applications. If it can't, we may conditionally switch between sending SIGTERM on non-windows systems, and sending SIGBREAK or whatever leads to graceful shutdown on windows.

@CMCDragonkai
Copy link
Member

Fixing 1. is easy.

As for 2. please do some tests on matrix-win-1 as to which signals can be caught when sending signals to them. Not through Ctrl + C on the powershell, but through another nodejs process sending SIGINT, SIGHUP... etc. Because that's what the tests are doing.

The powershell/cmd terminals may be doing something different when doing Ctrl + C.

@CMCDragonkai CMCDragonkai changed the title Signal handling for windows Signal Handling for windows Jul 29, 2022
@CMCDragonkai CMCDragonkai added the r&d:polykey:supporting activity Supporting core activity label Jul 10, 2023
@CMCDragonkai CMCDragonkai transferred this issue from MatrixAI/Polykey Aug 11, 2023
@tegefaulkes tegefaulkes removed their assignment Jul 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development Standard development r&d:polykey:supporting activity Supporting core activity
Development

No branches or pull requests

2 participants