feat(runtime/signal): implement SIGINT and SIGBREAK for windows #14694
Conversation
|
@bartlomieju The type errors for sigbreak are gone now, but other tests are failing. Maybe that's because of the std update? |
Yes, that's true. I'll make sure to upgrade |
|
Opened #14722 |
|
@GJZwiers after merging with |
| "SIGBREAK" => CTRL_BREAK_EVENT, | ||
| _ => unreachable!(), | ||
| }, | ||
| pid as u32, |
There was a problem hiding this comment.
@GJZwiers Did you check that processes actually receive the CTR_C_EVENT?
According to MSDN (which is often wildly inaccurate, but still) you can only send it to CTR_C_EVENT to all processes in the terminal.
From: https://docs.microsoft.com/en-us/windows/console/generateconsolectrlevent
This signal cannot be limited to a specific process group. If dwProcessGroupId is nonzero, this function will succeed, but the CTRL+C signal will not be received by processes within the specified process group.
There was a problem hiding this comment.
I checked some of the examples on https://deno.land/manual/examples/os_signals and they are working on windows with the current changes.
There was a problem hiding this comment.
I was trying to write some tests for SIGINT and SIGBREAK in this PR and ran into this issue where sending a SIGINT to the child process also kills the parent in the terminal. It seems very unexpected and different than other OSes.
There was a problem hiding this comment.
For WinOS, new "child" processes, created from a console process using CreateProcess(), all share the same console (unless created with CREATE_NEW_CONSOLE or DETACHED_PROCESS). CTRL+C and CTRL+BREAK events are sent to all processes which share that console, so all processes in the console process group will receive the event/signal.
For situations requiring shared I/O with a "parent", such as a shim, I'll just use null handlers for signals in the parent while waiting for the "child" process to finish.
ref: MS ~ Creation of a Console @@ https://archive.is/yFRT5
ref: MS ~ CTRL+C and CTRL+BREAK Signals @@ https://archive.is/6T140
runtime/ops/process.rs
Outdated
| let handle = unsafe { OpenProcess(PROCESS_TERMINATE, FALSE, pid as DWORD) }; | ||
| if pid <= 0 { | ||
| return Err(type_error( | ||
| "Process id (pid) cannot be negative or zero on Windows.", |
There was a problem hiding this comment.
Wrt CTRL_C_EVENT and CTRL_BREAK_EVENT: Windows supports sending them to pid 0.
There was a problem hiding this comment.
TIL :) made some changes
| match self { | ||
| WindowsSignal::Sigint(ctrl_c) => ctrl_c.recv().await, | ||
| WindowsSignal::Sigbreak(ctrl_break) => ctrl_break.recv().await, | ||
| } |
There was a problem hiding this comment.
We could also listen to CTRL_CLOSE_EVENT and map it to SIGHUP. Node does this too.
There was a problem hiding this comment.
There doesn't seem to be a type for ctrl_close in tokio::signal::windows though
…/deno into sigint_sigbreak_windows
|
@GJZwiers , thanks for this work! This only partially fixes #9995 (which is requesting broad signal handler parity for WinOS). It definitely goes a long way toward the goal by implementing the most commonly needed signals ('SIGBREAK' and 'SIGINT'). But, I wouldn't close #9995 based on the partial implementation. |
Ah my bad, I'll update the description |
There was a problem hiding this comment.
LGTM, that's a great improvement @GJZwiers!
Note to self: remember to credit https://github.com/juzi5201314 when landing
Revival of #10258. Adds support for SIGINT and SIGBREAK signals on Windows platform.
std/node/with SIGBREAKLots of credits to @juzi5201314 for the initial efforts.
Closes #10236.