-
-
Notifications
You must be signed in to change notification settings - Fork 2.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
Fix CTRL_CLOSE, CTRL_LOGOFF and CTRL_SHUTDOWN on windows #7122
Conversation
77cf12e
to
6e60886
Compare
…program before it had the chance to react
6e60886
to
44a84f2
Compare
Co-authored-by: Alice Ryhl <[email protected]>
Thanks for doing this. I tested this with CTRL_CLOSE and it works for me. FWIW, this is the same solution that libuv does: https://github.com/libuv/libuv/blob/23632e9104b6506d5bee0d85b807c2351617d226/src/win/signal.c#L130 |
@dsherret Actually, it doesn't. It only sleeps if someone handles the event, we should do that as well. |
@dsherret would you mind testing the PR again now? |
Yup, still works. |
@Darksonn Ready for another review |
Why do we handle all three this way when libuv only does it for one of them? |
Why does libuv only handle one? |
@Darksonn Either way, the other two signals are completely pointless for Tokio if we don't handle them like this, according to the windows api docs. They also result in an immediate kill after returning from the handler. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, just some comments on tests.
cc @ipetkov please review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have the means to test this myself, but since others have commented this works for them I'll trust that its helping!
I can test it tomorrow if needed |
Yes, please. |
Maybe I'm just too tired right now, but shouldn't this work?: |
What's the actual behaviour? (I'm AFK atm. My guess is it creates a single file and exits, which is fine I think?) |
Yes, it should create 4-5 files and then be killed. I'm not sure why, but in my own tests it seemed to work in cmd, but not in powershell. |
@Astlaan is it the same behavior I described here? #7039 (comment) |
Yes, it works for me. By the way, that example correctly creates 4-5 files on both cmd and powershell from windows terminal and windows console host. Also, I see now that 4-5 files is the expected behaviour because there's a 5000ms timeout before windows kills the process. |
Are you using my code? Because for me I see no files being created at all. |
Yup. What terminal are you using? |
I tried with both cmd and powershell. |
Try Also, is cmd and powershell being launched with windows console host? Does it work for you in windows terminal with cmd and powershell? I think windows console host/windows terminal is what sends the event. |
This was the issue indeed. It runs fine if I run it directly. |
In that case, ready for merging, @Darksonn. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you.
Fixes #7039.
Motivation
According to https://learn.microsoft.com/en-us/windows/console/handlerroutine#remarks, the windows signals
CTRL_CLOSE
,CTRL_LOGOFF
andCTRL_SHUTDOWN
immediately kill the process upon being handled.The intention is that the handler of those signals performs cleanup tasks, and once completed, it returns and the process is killed.
This behavior is sadly not really compatible with an async system that is based on signals.
Solution
There are multiple possible ways to solve this; the easiest one being to simply never return from the handler thread. This works because Rust calls
ExitProcess
after its main (to my knowledge). I tested it on a project of mine, and it seems to work indeed.