-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Port signals to Windows #7339
Comments
Possible replacements for SIGSTOP and SIGCONT:
|
Bear in mind, |
Good news: WM_CLOSE, TerminateProcess and DebugActiveProcess work like a charm. I wrote a simple C++ console app for testing purposes: https://github.com/maltevoos/SignalFaker Bad news: GenerateConsoleCtrlEvent doesn't work at all. It even says in the docs:
Probably should have looked at this closer. @sancarn I know this only works for windowed processes, but considering that it works very well for those and there is no real alternative that works for any process (as far as I know) I think this is the way to go. Of course we need to make it clear in the docs that SIGTERM doesn't work for console processes on Windows. Or we could call our SIGINT replacement instead when the user tries to send a SIGTERM to a console app. |
@maltevoos If so then you might consider using |
@sancarn So what exactly is the difference between WM_CLOSE and WM_QUIT and what makes one better? Btw, a short explanation of the signals I mentioned:
|
Turns out that I was mistaken and |
I think we should stick with WM_CLOSE because this is what taskkill uses (at least multiple stackoverflow posts say so). |
Are you sure
or for the current process use Edit: Also found an article about it which provides full example. |
https://github.com/jruby/jruby/blob/c3385b8feac7c05675a5f430b0d9b79f0edc5d37/core/src/main/java/org/jruby/RubyProcess.java#L1286 is how jruby does it, well for 0 and 9 anyway.
And here's how ruby does it https://github.com/ruby/ruby/blob/6a65f2b1e479f268489b51a004b6c153c634c68a/win32/win32.c#L4881 0,2,9 Maybe 15 could be WM_CLOSE if it's windowed, else |
@sancarn That looks pretty promising indeed. Will try it out later today :) @rdp Edit: Apparently the
But AFAIK the Ctrl-C event should work just fine in any case so I'll try that first. |
My hunch is they always send ctrl+break so that it works for "both pids and process groups" in case they happen to be targeting a process group? Kind of weird... Also, appears one needs to call AttachConsole before sending the ctrl+break but...then you lose your current console, not to mention temporarily losing your own ctrl+c handler (see "cons" listed here https://stackoverflow.com/questions/813086/can-i-send-a-ctrl-c-sigint-to-an-application-on-windows ). Maybe we should just shell out to taskkill ... ? :/ |
@rdp See #7339 (comment) |
@rdp |
@sancarn I think one of the "cons" is that after running code like that you would no longer be bound to your initial console (as in...printf would now go no where...)? @maltevoos dang...wonder if there is any built-in way to "shell out" to something that sends a ctrl+c signal? I couldn't find any...I guess we could tell people "if you want this to work, you have to bundle it with this helper exe" :| |
Unrelated to the standard linux signals, I wonder if there's a way to add handlers for other windows signals, like CTRL_LOGOFF_EVENT https://docs.microsoft.com/en-us/windows/console/handlerroutine or maybe it's already possible dunno. Might be useful long term anyway, FWIW :) |
@rdp I agree. We can't accept losing our console window just for terminating another process. Additionally, GenerateConsoleCtrlEvent doesn't do the same as a real Ctrl-C user input. For example when Ctrl-C-ing node, it always prompts the user to press Ctrl-C again. When using GenerateConsoleCtrlEvent, it prints out "^C", but node stops immediately. |
I meant to comment on this issue a few days ago, but got bogged down. I had been looking into this because I wanted to help port My one suggestion is that we provide support only for the the signals that Windows itself "supports" based on the latest docs.
Windows uses Structured Exception Handling, and so we can't really catch stuff like SIGFPE and SIGSEGV unless we wrap If we take the route of only supporting the above signals, we can be very clear about this in the documentation and set expectations early. The Windows story for signals across newer languages seems shaky. It isn't clear what exactly Go implements for Windows. It seems to only implement the CTRL-C stuff. It does not implement Rust doesn't seem to implement anything for Windows in the standard library. The tutorial suggests using a 3rd party library, and there's an RFC to add signal handling to the stdlib, but it's still open, and the latest reply suggests using a 3rd party library. Python handles things like I've mentioned above (and this seems like the sanest approach). It's much less work than trying to find replacements for all the UNIX signals, especially when we can't necessarily guarantee that they'll always behave the same way (through changing system APIs, etc.) It's less maintenance going forward, and I'd argue that the Python signal handling presents a more concise and coherent API surface for someone using the language. Edit: Wording. |
@markrjr Catching signals on Windows works the same way as on UNIX, namely through Edit: Apparently Windows does use the signals it supports (like SIGINT or SIGABRT). Looks like I was mistaken there. |
@markrjr The Python devs only did the easy part, namely registering signal handlers for the signals supported by windows when the user calls
Why Python's way of doing this is bad:
Sadly, most languages with signal "support" on Windows do it this way. Crystal should do better 🥇 |
@rdp > I think one of the "cons" is that after running code like that you would no longer be bound to your initial console (as in...printf would now go no where...)? I don't think that is true, because I assume Crystal will implement |
@sancarn I tested it yesterday. It doesn't work. Edit 2: I tested it again with AllocConsole but this opens an entirely new console window which looks very weird. Another approach that came to my mind:
Another nice thing about this would be that we probably don't need a |
I didn't read the entire conversation, but in my opinion signals in Crystal should be done like in Go: only References: |
@asterite I disagree. In my opinion we should support any signal that can be elegantly replaced with a WinApi call.
The functions above are tested and (almost) work like real signals on UNIX. |
@maltevoos There's an argument about maintenance overhead in supporting these replacements - how big would it be (LOC-wise), what's the complexity of OS-support matrix, how many edge-cases (like I'd go for sure for all of the cases we can support transparently, and weighted the rest against above argument. |
@Sija IMO it is worth it:
(lines of naive C++ code, each signal only takes a PID like on UNIX) Probably there is some more work to do if we want good error handling etc. |
I misunderstood, the blurb about structured exception handling is only important with OS -> Process communication, but since we're looking for IPC then we'd probably want to use the Messages API. @maltevoos It looks like you've already experimented with the Messages API for |
We could possibly use |
@markrjr I don't think we should invest time into this. I mean, what's the point of having two Crystal processes (that must be GUI processes) running at the same time and communicating to each other? We should keep focusing on sending signals to external processes. |
It's not much work/overhead, and makes it so you don't have to remember/use a "different way to kill process based on the current OS" as it were...I like maltevoos' roadmap (though who uses SIGCONT etc. in real life, but maybe still useful). I believe you can catch SIGFPE etc. but you can't "send" it to another process AFAICT. |
AFAIK only signals that exist on the target are defined. I.e. Signal's enum should only contain valid signals, not fake ones. On POSIX systems you get all signals, on Windows you have the few supported ones. Overall, only Interrupt and Kill are guaranteed to be available. Using other signals isn't portable. IMO this is a good compromise. It breaks the "code must compile on all targets" but using signals shouldn't be recommended, and usually not needed. |
So how do you portably use even these two? Is it possible to |
For the record, the value SIGKILL doesn't exist anywhere in Windows API. |
"Sending a signal" in general is not a thing on Windows. |
As was stated previously:
|
That's already answered in the issue description. And the same thing as Go does: When win32 So for a MVP I'd suggest to only implement custom handling of SIGKILL on windows for now. |
That's the obvious part. So then, what should the value of Signal::KILL be on Windows? |
Why not introduce like |
We are indeed moving in that direction, so perhaps we wouldn't need to fake-implement signals, but this is not a universal solution. |
( |
I really like what go does, in that it defines a list of signals, and all the signal values are always there on every OS, even if using them is an error. That go has the same guarantees that compiling always works on every OS and any failures happen at runtime is a really good sign. But only providing 2 enum values is not feasible in crystal. It's a simple extension of go's approach to add more signal values than just two. |
-- all two of them - Interrupt and Kill. I'm not sure how commendable that is |
@RX14 To use signals you must use values from the For example, $ GOOS=linux bin/go build foo.go $ GOOS=windows bin/go build foo.go
./foo.go:14:28: undefined: syscall.SIGTTOU I.e. using signals easily breaks Go programs portability. It's a good demonstration that the "compiles everywhere" policy is meant as a goal that shouldn't be enforced blindly —let developers shoot themselves in some corners, but make it explicit. In Crystal terminology, this means that there isn't a |
I'm fine with that general approach, since it's obvious that In fact I'd like to move |
Neither Putting LibC is also nonsensical (especially if we're meant to access signal constants from LibC). It's not unstable. It's unsafe and not portable but the C API is very stable. Let's remember that easy interaction with C API is one of the very powerful feature of Crystal... |
@ysbaddaden This goes a bit off topic, but just to clarify: The Crystal namespace actually provides some features that are definitely intended for usage outside stdlib. Most inside that namespace is not intended as public API and thus nodoc, though. Especially everything in |
|
That suggestion could be nice, it'd expose usages of Still, this wouldn't get us closer to the solution of the problem at hand. |
Not sure it helps, but here's what Rust says about Signals. |
IMO we should aim for implementating the use cases of Terminating a process
Handling
|
This issue is about porting
Signal
to Windows and therefore making it possible to implement other relevant features likeProcess
.Short introduction (moved from #5430):
Microsft doc about signals
I already looked at this page, and I think these signals are only there because they are part of the C standard library. From wikipedia:
And these 6 signals are the only ones "supported" by Windows.
There is already an issue about this topic on the Node.js repo.
sam-github commented there:
And I guess this is exactly the case. You have a function for listening to signals (
signal()
), but there is nothing likekill()
for sending signals on Windows.So this is about finding replacement functions to "fake" signals on Windows. I already found some replacements for signals one might want to send:
SendMessage(WM_CLOSE)
<- this is also whattaskkill.exe
uses; works for windowed processesTerminateProcess()
<- used bytaskkill.exe /f
; works for windowed and console processesGenerateConsoleCtrlEvent()
<- works for console processes, simulates Ctrl + CList of UNIX signals: https://en.wikipedia.org/wiki/Signal_(IPC)
Another microsoft doc that mentions replacements for UNIX signals: https://docs.microsoft.com/en-us/previous-versions/ms811896(v=msdn.10)#signals-and-signal-handling
The text was updated successfully, but these errors were encountered: