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

Add empty ctrlc handler on Windows. #6002

Closed
wants to merge 1 commit into from

Conversation

zachlute
Copy link
Contributor

Fixes #6000.

When exec_replacing the cargo process, we want the new process to get any signals. This already works fine on Unix.

On Windows, pressing ctrlc kills the cargo process and doesn't pass the signal on to the child process. By adding an empty handler, we allow the child process to handle the signal instead.

When exec_replacing the cargo process, we want the new process to get any signals. This already works fine on Unix.

On Windows, pressing ctrlc kills the cargo process and doesn't pass the signal on to the child process. By adding an empty handler, we allow the child process to handle the signal instead.
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matklad (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@alexcrichton
Copy link
Member

Thanks for the PR here! I think this is a good solution, and I learned something new about Windows ctrl-c handlers today!

Could this perhaps just use winapi though? It looks like the ctrlc crate spawns a helper thread whenever it's called but we don't actually need that to execute because it's a noop function anyway. Additionally, a comment above the function be added along the lines of:

In exec_replace we ideally want to replace our current process with the target process, but on Windows this isn't technically possible. Instead we emulate it to the best of our ability.

One aspect we fix here is that we specify a handler for the ctrl-c handler. In doing so (and by effectively ignoring it) we should emulate proxying ctrl-c handling to the application at hand, which will either terminate or handle it itself. According to microsoft's documentation -- https://docs.microsoft.com/en-us/windows/console/ctrl-c-and-ctrl-break-signals -- the ctrl-c signal is sent to all processes attached to a terminal, which should include our child process. If the child terminates then we'll reap them in Cargo pretty quickly, and if the child handles the signal then we won't terminate (and we shouldn't!) until the process itself later exits.

Note that this should mirror behavior on Unix where Cargo literally uses exec so the way a ctrl-c is dealt with depends on the process that's running.

@alexcrichton
Copy link
Member

Oh also, could a test using GetConsoleCtrlEvent be added to test this?

@zachlute
Copy link
Contributor Author

I think you're probably right that ctrlc is overkill for this particular case. I'll take a poke at it and see if I can accomplish the same thing with just the winapi calls.

@zachlute
Copy link
Contributor Author

Going to close this PR in favor PR #6004 which does the same thing but without the ctrlc depdency.

@zachlute zachlute closed this Sep 10, 2018
bors added a commit that referenced this pull request Sep 15, 2018
Add empty ctrlc handler on Windows.

Fixes #6000.

This is a 'better' version of PR #6002 that accomplishes the same thing using only `winapi` calls without the dependency on the `ctrlc` crate or spawning an additional thread.

----

When exec_replacing the cargo process, we want the new process to get any signals. This already works fine on Unix.

On Windows, pressing ctrlc kills the cargo process and doesn't pass the signal on to the child process. By adding an empty handler, we allow the child process to handle the signal instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants