Auto merge of #2370 - alexcrichton:windows-jobs, r=brson

bors · bors · commit 4b7f9dfef205 · 2016-02-11T19:51:06.000Z
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
diff --git a/appveyor.yml b/appveyor.yml
@@ -5,7 +5,6 @@ environment:
     BITS: 32
     TARGET: i686-pc-windows-msvc
     ARCH: x86
-    NEEDS_LIBGCC: 1
   - MSVC: 1
     BITS: 64
     TARGET: x86_64-pc-windows-msvc
@@ -17,14 +16,13 @@ install:
   - call "C:\Program Files (x86)\Microsoft Visual Studio 12.0\VC\vcvarsall.bat" %ARCH%
   - SET PATH=%PATH%;%cd%/rustc/bin
   - SET PATH=%PATH%;%cd%/target/snapshot/bin
-  - if defined NEEDS_LIBGCC set PATH=%PATH%;C:\MinGW\bin
   - rustc -V
   - cargo -V
 
 build: false
 
 test_script:
-  - cargo test -- --nocapture
+  - cargo test
 
 branches:
   only:
diff --git a/src/bin/cargo.rs b/src/bin/cargo.rs
@@ -104,6 +104,7 @@ fn execute(flags: Flags, config: &Config) -> CliResult<Option<()>> {
     try!(config.shell().set_color_config(flags.flag_color.as_ref().map(|s| &s[..])));
 
     init_git_transports(config);
+    cargo::util::job::setup();
 
     if flags.flag_version {
         println!("{}", cargo::version());
diff --git a/src/cargo/util/job.rs b/src/cargo/util/job.rs
@@ -0,0 +1,91 @@
+//! Job management (mostly for windows)
+//!
+//! Most of the time when you're running cargo you expect Ctrl-C to actually
+//! terminate the entire tree of processes in play, not just the one at the top
+//! (cago). This currently works "by default" on Unix platforms because Ctrl-C
+//! actually sends a signal to the *process group* rather than the parent
+//! process, so everything will get torn down. On Windows, however, this does
+//! not happen and Ctrl-C just kills cargo.
+//!
+//! To achieve the same semantics on Windows we use Job Objects to ensure that
+//! all processes die at the same time. Job objects have a mode of operation
+//! where when all handles to the object are closed it causes all child
+//! processes associated with the object to be terminated immediately.
+//! Conveniently whenever a process in the job object spawns a new process the
+//! child will be associated with the job object as well. This means if we add
+//! ourselves to the job object we create then everything will get torn down!
+
+pub fn setup() {
+    unsafe { imp::setup() }
+}
+
+#[cfg(unix)]
+mod imp {
+    use std::env;
+    use libc;
+
+    pub unsafe fn setup() {
+        // There's a test case for the behavior of
+        // when-cargo-is-killed-subprocesses-are-also-killed, but that requires
+        // one cargo spawned to become its own session leader, so we do that
+        // here.
+        if env::var("__CARGO_TEST_SETSID_PLEASE_DONT_USE_ELSEWHERE").is_ok() {
+            libc::setsid();
+        }
+    }
+}
+
+#[cfg(windows)]
+mod imp {
+    extern crate kernel32;
+    extern crate winapi;
+
+    use std::mem;
+
+    pub unsafe fn setup() {
+        // Creates a new job object for us to use and then adds ourselves to it.
+        // Note that all errors are basically ignored in this function,
+        // intentionally. Job objects are "relatively new" in Windows,
+        // particularly the ability to support nested job objects. Older
+        // Windows installs don't support this ability. We probably don't want
+        // to force Cargo to abort in this situation or force others to *not*
+        // use job objects, so we instead just ignore errors and assume that
+        // we're otherwise part of someone else's job object in this case.
+
+        let job = kernel32::CreateJobObjectW(0 as *mut _, 0 as *const _);
+        if job.is_null() {
+            return
+        }
+
+        // Indicate that when all handles to the job object are gone that all
+        // process in the object should be killed. Note that this includes our
+        // entire process tree by default because we've added ourselves and and
+        // our children will reside in the job once we spawn a process.
+        let mut info: winapi::JOBOBJECT_EXTENDED_LIMIT_INFORMATION;
+        info = mem::zeroed();
+        info.BasicLimitInformation.LimitFlags =
+            winapi::JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE;
+        let r = kernel32::SetInformationJobObject(job,
+                        winapi::JobObjectExtendedLimitInformation,
+                        &mut info as *mut _ as winapi::LPVOID,
+                        mem::size_of_val(&info) as winapi::DWORD);
+        if r == 0 {
+            kernel32::CloseHandle(job);
+            return
+        }
+
+        // Assign our process to this job object, meaning that our children will
+        // now live or die based on our existence.
+        let me = kernel32::GetCurrentProcess();
+        let r = kernel32::AssignProcessToJobObject(job, me);
+        if r == 0 {
+            kernel32::CloseHandle(job);
+            return
+        }
+
+        // Intentionally leak the `job` handle here. We've got the only
+        // reference to this job, so once it's gone we and all our children will
+        // be killed. This typically won't happen unless Cargo itself is
+        // ctrl-c'd.
+    }
+}
diff --git a/src/cargo/util/mod.rs b/src/cargo/util/mod.rs
@@ -29,6 +29,7 @@ pub mod to_semver;
 pub mod to_url;
 pub mod toml;
 pub mod lev_distance;
+pub mod job;
 mod dependency_queue;
 mod sha256;
 mod shell_escape;
diff --git a/tests/test_cargo_death.rs b/tests/test_cargo_death.rs
@@ -0,0 +1,108 @@
+use std::net::TcpListener;
+use std::io::{self, Read};
+use std::process::{Stdio, Child};
+
+use support::project;
+
+fn setup() {}
+
+#[cfg(unix)]
+fn enabled() -> bool {
+    true
+}
+
+// On Windows suport for these tests is only enabled through the usage of job
+// objects. Support for nested job objects, however, was added in recent-ish
+// versions of Windows, so this test may not always be able to succeed.
+//
+// As a result, we try to add ourselves to a job object here
+// can succeed or not.
+#[cfg(windows)]
+fn enabled() -> bool {
+    use kernel32;
+    use winapi;
+    unsafe {
+        // If we're not currently in a job, then we can definitely run these
+        // tests.
+        let me = kernel32::GetCurrentProcess();
+        let mut ret = 0;
+        let r = kernel32::IsProcessInJob(me, 0 as *mut _, &mut ret);
+        assert!(r != 0);
+        if ret == winapi::FALSE {
+            return true
+        }
+
+        // If we are in a job, then we can run these tests if we can be added to
+        // a nested job (as we're going to create a nested job no matter what as
+        // part of these tests.
+        //
+        // If we can't be added to a nested job, then these tests will
+        // definitely fail, and there's not much we can do about that.
+        let job = kernel32::CreateJobObjectW(0 as *mut _, 0 as *const _);
+        assert!(!job.is_null());
+        let r = kernel32::AssignProcessToJobObject(job, me);
+        kernel32::CloseHandle(job);
+        r != 0
+    }
+}
+
+test!(ctrl_c_kills_everyone {
+    if !enabled() {
+        return
+    }
+
+    let listener = TcpListener::bind("127.0.0.1:0").unwrap();
+    let addr = listener.local_addr().unwrap();
+
+    let p = project("foo")
+        .file("Cargo.toml", r#"
+            [package]
+            name = "foo"
+            version = "0.0.1"
+            authors = []
+            build = "build.rs"
+        "#)
+        .file("src/lib.rs", "")
+        .file("build.rs", &format!(r#"
+            use std::net::TcpStream;
+            use std::io::Read;
+
+            fn main() {{
+                let mut socket = TcpStream::connect("{}").unwrap();
+                let _ = socket.read(&mut [0; 10]);
+                panic!("that read should never return");
+            }}
+        "#, addr));
+    p.build();
+
+    let mut cargo = p.cargo("build").build_command();
+    cargo.stdin(Stdio::piped())
+         .stdout(Stdio::piped())
+         .stderr(Stdio::piped())
+         .env("__CARGO_TEST_SETSID_PLEASE_DONT_USE_ELSEWHERE", "1");
+    let mut child = cargo.spawn().unwrap();
+
+    let mut sock = listener.accept().unwrap().0;
+    ctrl_c(&mut child);
+
+    assert!(!child.wait().unwrap().success());
+    match sock.read(&mut [0; 10]) {
+        Ok(n) => assert_eq!(n, 0),
+        Err(e) => assert_eq!(e.kind(), io::ErrorKind::ConnectionReset),
+    }
+});
+
+#[cfg(unix)]
+fn ctrl_c(child: &mut Child) {
+    use libc;
+
+    let r = unsafe { libc::kill(-(child.id() as i32), libc::SIGINT) };
+    if r < 0 {
+        panic!("failed to kill: {}", io::Error::last_os_error());
+    }
+}
+
+#[cfg(windows)]
+fn ctrl_c(child: &mut Child) {
+    child.kill().unwrap();
+}
diff --git a/tests/tests.rs b/tests/tests.rs
@@ -69,6 +69,7 @@ mod test_cargo_tool_paths;
 mod test_cargo_verify_project;
 mod test_cargo_version;
 mod test_shell;
+mod test_cargo_death;
 
 thread_local!(static RUSTC: Rustc = Rustc::new("rustc").unwrap());
 
