Use job objects on windows for ctrl-c to work
authorAlex Crichton <alex@alexcrichton.com>
Mon, 8 Feb 2016 19:28:53 +0000 (11:28 -0800)
committerAlex Crichton <alex@alexcrichton.com>
Thu, 11 Feb 2016 19:49:45 +0000 (11:49 -0800)
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

appveyor.yml
src/bin/cargo.rs
src/cargo/util/job.rs [new file with mode: 0644]
src/cargo/util/mod.rs
tests/test_cargo_death.rs [new file with mode: 0644]
tests/tests.rs

index 39c11e1f7c82f0375c5a1ee3eb6e936e8a694fde..a6009fa5f1e7910bdd0e7fd634aacbfcf75a8ae1 100644 (file)
@@ -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:
index e37486d9b33945d897cc658fd73e1d95ccc20d0a..23941b50e3941e5b4cfb1c2d208fbaeabbf2c8dc 100644 (file)
@@ -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
new file mode 100644 (file)
index 0000000..dee6d0b
--- /dev/null
@@ -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.
+    }
+}
index 49963201d926efd08525e93d00665741385728b6..c83356767aefee2e8dcb929fec798ff85c34b7e0 100644 (file)
@@ -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
new file mode 100644 (file)
index 0000000..3233a5c
--- /dev/null
@@ -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();
+}
index 52e602507e70621eef59c2195e950e1c2e99ccc2..711bf04c6832628910fb99c538706a60934629fc 100644 (file)
@@ -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());