Auto merge of #8162 - ehuss:linking-interrupted-flaky, r=alexcrichton

Fix flaky linking_interrupted test.

The `linking_interrupted` test is a little flaky, and can randomly fail.

On windows-gnu, the last call to `cargo test` was failing because it was hitting a file lock. Presumably this is because the `kill` had not finished? Added a wait to ensure that cargo has exited.

Also, sometimes on Windows, the rustc process gets killed. Not sure why that doesn't happen all the time, maybe related to the race mentioned above.

Finally, on macOS, I was noticing some errors where I think the rustc process had not yet exited, and deleted the `*.o` files while the next cargo build was running.

Author: bors

tests/testsuite/death.rs

@@ -8,52 +8,8 @@ use std::thread;
 
 use cargo_test_support::{project, slow_cpu_multiplier};
 
-#[cfg(unix)]
-fn enabled() -> bool {
-    true
-}
-
-// On Windows support 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 winapi::um::{handleapi, jobapi, jobapi2, processthreadsapi};
-
-    unsafe {
-        // If we're not currently in a job, then we can definitely run these
-        // tests.
-        let me = processthreadsapi::GetCurrentProcess();
-        let mut ret = 0;
-        let r = jobapi::IsProcessInJob(me, 0 as *mut _, &mut ret);
-        assert_ne!(r, 0);
-        if ret == ::winapi::shared::minwindef::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 = jobapi2::CreateJobObjectW(0 as *mut _, 0 as *const _);
-        assert!(!job.is_null());
-        let r = jobapi2::AssignProcessToJobObject(job, me);
-        handleapi::CloseHandle(job);
-        r != 0
-    }
-}
-
 #[cargo_test]
 fn ctrl_c_kills_everyone() {
-    if !enabled() {
-        return;
-    }
-
     let listener = TcpListener::bind("127.0.0.1:0").unwrap();
     let addr = listener.local_addr().unwrap();
 
@@ -132,14 +88,14 @@ fn ctrl_c_kills_everyone() {
 }
 
 #[cfg(unix)]
-fn ctrl_c(child: &mut Child) {
+pub fn ctrl_c(child: &mut Child) {
     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) {
+pub fn ctrl_c(child: &mut Child) {
     child.kill().unwrap();
 }

tests/testsuite/freshness.rs

@@ -10,6 +10,7 @@ use std::process::Stdio;
 use std::thread;
 use std::time::SystemTime;
 
+use super::death;
 use cargo_test_support::paths::{self, CargoPathExt};
 use cargo_test_support::registry::Package;
 use cargo_test_support::{basic_manifest, is_coarse_mtime, project, rustc_host, sleep_ms};
@@ -2316,8 +2317,14 @@ LLVM version: 9.0
 fn linking_interrupted() {
     // Interrupt during the linking phase shouldn't leave test executable as "fresh".
 
-    let listener = TcpListener::bind("127.0.0.1:0").unwrap();
-    let addr = listener.local_addr().unwrap();
+    // This is used to detect when linking starts, then to pause the linker so
+    // that the test can kill cargo.
+    let link_listener = TcpListener::bind("127.0.0.1:0").unwrap();
+    let link_addr = link_listener.local_addr().unwrap();
+
+    // This is used to detect when rustc exits.
+    let rustc_listener = TcpListener::bind("127.0.0.1:0").unwrap();
+    let rustc_addr = rustc_listener.local_addr().unwrap();
 
     // Create a linker that we can interrupt.
     let linker = project()
@@ -2326,8 +2333,6 @@ fn linking_interrupted() {
         .file(
             "src/main.rs",
             &r#"
-            use std::io::Read;
-
             fn main() {
                 // Figure out the output filename.
                 let output = match std::env::args().find(|a| a.starts_with("/OUT:")) {
@@ -2346,43 +2351,79 @@ fn linking_interrupted() {
                 std::fs::write(&output, "").unwrap();
                 // Tell the test that we are ready to be interrupted.
                 let mut socket = std::net::TcpStream::connect("__ADDR__").unwrap();
-                // Wait for the test to tell us to exit.
-                let _ = socket.read(&mut [0; 1]);
+                // Wait for the test to kill us.
+                std::thread::sleep(std::time::Duration::new(60, 0));
             }
             "#
-            .replace("__ADDR__", &addr.to_string()),
+            .replace("__ADDR__", &link_addr.to_string()),
         )
         .build();
     linker.cargo("build").run();
 
+    // Create a wrapper around rustc that will tell us when rustc is finished.
+    let rustc = project()
+        .at("rustc-waiter")
+        .file("Cargo.toml", &basic_manifest("rustc-waiter", "1.0.0"))
+        .file(
+            "src/main.rs",
+            &r#"
+            fn main() {
+                let mut conn = None;
+                // Check for a normal build (not -vV or --print).
+                if std::env::args().any(|arg| arg == "t1") {
+                    // Tell the test that rustc has started.
+                    conn = Some(std::net::TcpStream::connect("__ADDR__").unwrap());
+                }
+                let status = std::process::Command::new("rustc")
+                    .args(std::env::args().skip(1))
+                    .status()
+                    .expect("rustc to run");
+                std::process::exit(status.code().unwrap_or(1));
+            }
+            "#
+            .replace("__ADDR__", &rustc_addr.to_string()),
+        )
+        .build();
+    rustc.cargo("build").run();
+
     // Build it once so that the fingerprint gets saved to disk.
     let p = project()
         .file("src/lib.rs", "")
         .file("tests/t1.rs", "")
         .build();
     p.cargo("test --test t1 --no-run").run();
+
     // Make a change, start a build, then interrupt it.
     p.change_file("src/lib.rs", "// modified");
     let linker_env = format!(
         "CARGO_TARGET_{}_LINKER",
         rustc_host().to_uppercase().replace('-', "_")
     );
+    // NOTE: This assumes that the paths to the linker or rustc are not in the
+    // fingerprint. But maybe they should be?
     let mut cmd = p
         .cargo("test --test t1 --no-run")
         .env(&linker_env, linker.bin("linker"))
+        .env("RUSTC", rustc.bin("rustc-waiter"))
         .build_command();
     let mut child = cmd
         .stdout(Stdio::null())
         .stderr(Stdio::null())
+        .env("__CARGO_TEST_SETSID_PLEASE_DONT_USE_ELSEWHERE", "1")
         .spawn()
         .unwrap();
+    // Wait for rustc to start.
+    let mut rustc_conn = rustc_listener.accept().unwrap().0;
     // Wait for linking to start.
-    let mut conn = listener.accept().unwrap().0;
+    drop(link_listener.accept().unwrap());
 
     // Interrupt the child.
-    child.kill().unwrap();
-    // Note: rustc and the linker are still running, let them exit here.
-    conn.write(b"X").unwrap();
+    death::ctrl_c(&mut child);
+    assert!(!child.wait().unwrap().success());
+    // Wait for rustc to exit. If we don't wait, then the command below could
+    // start while rustc is still being torn down.
+    let mut buf = [0];
+    drop(rustc_conn.read_exact(&mut buf));
 
     // Build again, shouldn't be fresh.
     p.cargo("test --test t1")