Use job objects on windows for ctrl-c to work

[alexcrichton]

Currently it's somewhat surprising if you're using cargo and it's then ctrl-c'd.
The child processes that Cargo spawned are likely to still be running around in
the background as they're not killed as well, and this could cause output spew
or future build failures.

This situation is handled by default on Unix because ctrl-c will end up sending
a signal to the entire process group, which kills everything, but on Windows
we're not as lucky (just Cargo itself is killed). By using job objects on
Windows we can ensure that the entire tree dies instead of just the top Cargo
process.

cc #2343

[rust-highfive]

r? @wycats

(rust_highfive has picked a reviewer for you, use r? to override)

[alexcrichton]

r? @brson

[brson]

@bors r+

[bors]

📌 Commit 302bbba has been approved by brson

[bors]

⌛️ Testing commit 302bbba with merge 7823a5b...

[bors]

💔 Test failed - cargo-win-msvc-64

[alexcrichton]

Well, at least I can rest easy at night knowing that the test is indeed working?

Looks like our bots are running Window Server 2008 R2, which does not have support for nested jobs. We also run everything inside of dojob.exe which adds everything to a job object. So from this we can infer:

• Cargo does indeed continue to work if it can't put itself into a job object.
• The test added does indeed fail if job objects aren't working.

I... am not entirely sure what to do here. Maybe we can detect the Windows version and disable the test if Windows is too old? Maybe we can try adding ourself to two job objects and see if it works?

It looks like AppVeyor at least is running a newer version of Windows where the test can be run, so at least we have some coverage of it.

[bors]

⌛️ Testing commit 302bbba with merge 5b93d5c...

[alexcrichton]

@bors: r-

[alexcrichton]

Ok, I've added a check to only enable these tests if the tests are either (a) not in a job or (b) can add themselves to a nested job. I believe this means that buildbot tests will never run these tests (as the builders are already in a job and don't support nested jobs), but appveyor will run these tests (as will runs locally).

@bors: r=brson 5ccc842

[bors]

⌛️ Testing commit 5ccc842 with merge 4b7f9df...

[bors]

☀️ Test successful - cargo-linux-32, cargo-linux-64, cargo-mac-32, cargo-mac-64, cargo-win-gnu-32, cargo-win-gnu-64, cargo-win-msvc-32, cargo-win-msvc-64

[bruno-medeiros]

Cool! I was developing some IDE features that really heavily on this (invoking and killing cargo build automatically), and sometimes I'd get an error with a newer Cargo invocation, stating that it's waiting for the build subcommands to terminate (these would be lingering subcommands of a previous invocation).

I trust this will still work if the Cargo process is killed directly (programmatically), and not just when doing a Ctrl-C on the console? In other words, Cargo isn't trying to handle SIGINT or something?

[alexcrichton]

Indeed! Although on Unix you'll have to be sure to kill the process group rather than just the Cargo process. Other than that it should work just fine on Unix/Windows both programmatically and via ctrl-c

[bruno-medeiros]

Hum, I'm just using the Java API that spawns processes... I guess I will have to test how it actually works on Linux, see if it kills the whole process group. But from what I can see from a quick look, the only thing the Java API does on Linux is send a SIGTERM to the spawned process. I dunno if that is enough to kill the child processes of Cargo.

[alexcrichton]

Ah yeah unfortunately killing just Cargo isn't enough to kill child process on Unix, you'll have to kill the whole process group.

[bruno-medeiros]

Ok, that's good to know then. Although, it would be nicer if this functionality were built into Cargo itself. Maybe something for a PR..