Add empty ctrlc handler on Windows. #6004

zachlute · 2018-09-10T23:52:31Z

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.

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 · 2018-09-10T23:52:34Z

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.

zachlute · 2018-09-10T23:58:08Z

I would still like to add a test for this, but it's unclear how I would do so.

@alexcrichton suggested using GenerateConsoleCtrlEvent, but are there examples of tests right now that spawn an external cargo process and then manipulate it? Even if so, how would I know when it succeeded? It seems like it'd have to be some sort of three-tiered test where there's an executable that has a Ctrl-C handler that does something, and then the Cargo process runs that, and then the test calls the cargo process, sends it a Ctrl+C, and then verifies that the correct output happens.

But...uh...I think I need help figuring out how to set that up. :)

alexcrichton · 2018-09-11T17:29:39Z

@zachlute oh what I'm thinking is a test that does this:

First, this is a windows-specific test
A project is create with one binary. This binary sets a ctrl-c handler that prints something and then exits. Otherwise the binary in its main function prints a different marker and then sleeps forever.
We then run cargo run for this project.
When the binary prints the first marker, we kill the cargo process with GenerateConsoleCtrlEvent.
We then wait for everything to get torn down and assert that the output contains the marker printed by the ctrl-c handler.

I think that when cargo run is spawned we'll need to place it in a dedicated process group as well to avoid killing all the other tests.

Does that sounds possible?

zachlute · 2018-09-12T06:34:49Z

Yeah, that's pretty much what I was imagining, so good to hear confirmation I'm not ridiculous. I'll try to give it a go in the next couple of days. Thanks for the help!

zachlute · 2018-09-14T05:45:46Z

I'm beginning to believe writing a test for this is impossible. The basic issue is that GenerateConsoleCtrlEvent can't actually be sent to another process group:

Generates a CTRL+C signal. This signal cannot be generated for process groups. If dwProcessGroupId is nonzero, this function will succeed, but the CTRL+C signal will not be received by processes within the specified process group.

Things I have tried so far:

Creating a program with a Ctrl+C handler and sending GenerateConsoleCtrlEvent from the test. Kills the test.
Creating two programs, one with a Ctrl+C handler and another that attaches to that one's console and uses GenerateConsoleCtrlEvent to try to kill the process in the attached console.

The fundamental issue is that there's no way to run even the special killer process as NOT under the same console as the test, so no matter what I do, the event kills the test.

To do any of this at all required adding a way to spawn the runner process but not actually wait on it until we'd had a chance to run the second process, so that's in there, too.

You can view this abomination here in case you have any brilliant ideas, but I am out of them:

zachlute@1a4ca1d

zachlute · 2018-09-14T05:47:51Z

It's probably also worth noting that this broken test is horrifically slow, because it has to download and build the winapi stuff.

zachlute · 2018-09-14T06:47:02Z

FWIW, that issue appears to be a duplicate of #4575, so maybe @retep998 has some insight here as to:

Why my solution here may not work?
How to write a test for this solution, assuming it's okay.

alexcrichton · 2018-09-14T17:10:09Z

Oh dear, that really does make for a complicated function! I do think though that this'll be very valuable to have a test for! I'll take a crack at it tonight when I get back to a windows machine.

alexcrichton · 2018-09-15T00:21:58Z

Aha I'm now remembering something! Turns out on MSYS programs do not get a ctrl-c event. I think they're just killed with TerminateProcess.

This means that ctrl-c handlers (via SetConsoleCtrlHandler) just never get invoked for MSYS shells. This is why we've got job objects on windows to tear things down. I don't think that affects whether we want to do this, it just means that this still is mostly unimplemented for the msys shell (and only works with the cmd.exe shell I think)

alexcrichton

Ok I tried my damnednest and couldn't get a test working. The docs for GenerateConsoleCtrlEvent don't really make sense to me becuase I create a new process group and then had it send a signal to itself, but it still killed the main test.

In any case this is good enough to land I believe.

alexcrichton · 2018-09-15T00:56:13Z

src/cargo/util/process_builder.rs

+
+    pub fn exec_replace(process_builder: &ProcessBuilder) -> CargoResult<()> {
+        unsafe {
+            if SetConsoleCtrlHandler(Some(ctrlc_handler), TRUE) == FALSE {


I think this could probably use None to specify that the signal should be ignored, right?

You would think that, wouldn't you? But it doesn't work. What happens is that it EATS the Ctrl+C and it doesn't make it to the child process, so it's actually worse than the status quo because then you can't kill the cargo process OR the child process without task-killing it.

Ah ok, good to know!

alexcrichton · 2018-09-15T16:18:12Z

@bors: r+

bors · 2018-09-15T16:18:12Z

📌 Commit 6a3f4b9 has been approved by alexcrichton

bors · 2018-09-15T16:18:20Z

⌛ Testing commit 6a3f4b9 with merge 044a525...

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.

bors · 2018-09-15T17:03:06Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 044a525 to master...

Attempt to replicate the functionality from <rust-lang/cargo#6004> in order to pass Ctrl+C on in Windows Signed-off-by: Daniel Silverstone <dsilvers@digital-scurf.org>

rust-highfive assigned matklad Sep 10, 2018

zachlute mentioned this pull request Sep 10, 2018

Add empty ctrlc handler on Windows. #6002

Closed

alexcrichton reviewed Sep 15, 2018

View reviewed changes

bors merged commit 6a3f4b9 into rust-lang:master Sep 15, 2018

zachlute deleted the fix-ctrlc-handling2 branch September 15, 2018 19:26

zachlute mentioned this pull request Sep 15, 2018

With cargo run, forward termination signal to child process on Windows #4575

Closed

This was referenced Sep 21, 2019

Ctrl+C on cargo run #3281

Closed

Ctrl+C on cargo run rust-lang/rustup#806

Closed

ehuss added this to the 1.31.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add empty ctrlc handler on Windows. #6004

Add empty ctrlc handler on Windows. #6004

zachlute commented Sep 10, 2018

rust-highfive commented Sep 10, 2018

zachlute commented Sep 10, 2018

alexcrichton commented Sep 11, 2018

zachlute commented Sep 12, 2018

zachlute commented Sep 14, 2018

zachlute commented Sep 14, 2018

zachlute commented Sep 14, 2018

alexcrichton commented Sep 14, 2018

alexcrichton commented Sep 15, 2018

alexcrichton left a comment

alexcrichton Sep 15, 2018

zachlute Sep 15, 2018

alexcrichton Sep 15, 2018

alexcrichton commented Sep 15, 2018

bors commented Sep 15, 2018

bors commented Sep 15, 2018

bors commented Sep 15, 2018

Add empty ctrlc handler on Windows. #6004

Add empty ctrlc handler on Windows. #6004

Conversation

zachlute commented Sep 10, 2018

rust-highfive commented Sep 10, 2018

zachlute commented Sep 10, 2018

alexcrichton commented Sep 11, 2018

zachlute commented Sep 12, 2018

zachlute commented Sep 14, 2018

zachlute commented Sep 14, 2018

zachlute commented Sep 14, 2018

alexcrichton commented Sep 14, 2018

alexcrichton commented Sep 15, 2018

alexcrichton left a comment

Choose a reason for hiding this comment

alexcrichton Sep 15, 2018

Choose a reason for hiding this comment

zachlute Sep 15, 2018

Choose a reason for hiding this comment

alexcrichton Sep 15, 2018

Choose a reason for hiding this comment

alexcrichton commented Sep 15, 2018

bors commented Sep 15, 2018

bors commented Sep 15, 2018

bors commented Sep 15, 2018