Auto merge of #5887 - Seeker14491:job-fix, r=alexcrichton

 Don't kill child processes on normal exit on Windows

Fix for #5598

Author: bors

src/cargo/util/job.rs

@@ -44,22 +44,14 @@ mod imp {
 mod imp {
     extern crate winapi;
 
-    use std::ffi::OsString;
     use std::io;
     use std::mem;
-    use std::os::windows::prelude::*;
     use std::ptr;
 
-    use self::winapi::shared::basetsd::*;
     use self::winapi::shared::minwindef::*;
-    use self::winapi::shared::minwindef::{FALSE, TRUE};
     use self::winapi::um::handleapi::*;
     use self::winapi::um::jobapi2::*;
-    use self::winapi::um::jobapi::*;
     use self::winapi::um::processthreadsapi::*;
-    use self::winapi::um::psapi::*;
-    use self::winapi::um::synchapi::*;
-    use self::winapi::um::winbase::*;
     use self::winapi::um::winnt::*;
     use self::winapi::um::winnt::HANDLE;
 
@@ -93,7 +85,7 @@ mod imp {
 
         // 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
+        // entire process tree by default because we've added ourselves and
         // our children will reside in the job once we spawn a process.
         let mut info: JOBOBJECT_EXTENDED_LIMIT_INFORMATION;
         info = mem::zeroed();
@@ -121,24 +113,10 @@ mod imp {
 
     impl Drop for Setup {
         fn drop(&mut self) {
-            // This is a litte subtle. By default if we are terminated then all
-            // processes in our job object are terminated as well, but we
-            // intentionally want to whitelist some processes to outlive our job
-            // object (see below).
-            //
-            // To allow for this, we manually kill processes instead of letting
-            // the job object kill them for us. We do this in a loop to handle
-            // processes spawning other processes.
-            //
-            // Finally once this is all done we know that the only remaining
-            // ones are ourselves and the whitelisted processes. The destructor
-            // here then configures our job object to *not* kill everything on
-            // close, then closes the job object.
+            // On normal exits (not ctrl-c), we don't want to kill any child
+            // processes. The destructor here configures our job object to
+            // *not* kill everything on close, then closes the job object.
             unsafe {
-                while self.kill_remaining() {
-                    info!("killed some, going for more");
-                }
-
                 let mut info: JOBOBJECT_EXTENDED_LIMIT_INFORMATION;
                 info = mem::zeroed();
                 let r = SetInformationJobObject(
@@ -154,114 +132,6 @@ mod imp {
         }
     }
 
-    impl Setup {
-        unsafe fn kill_remaining(&mut self) -> bool {
-            #[repr(C)]
-            struct Jobs {
-                header: JOBOBJECT_BASIC_PROCESS_ID_LIST,
-                list: [ULONG_PTR; 1024],
-            }
-
-            let mut jobs: Jobs = mem::zeroed();
-            let r = QueryInformationJobObject(
-                self.job.inner,
-                JobObjectBasicProcessIdList,
-                &mut jobs as *mut _ as LPVOID,
-                mem::size_of_val(&jobs) as DWORD,
-                ptr::null_mut(),
-            );
-            if r == 0 {
-                info!("failed to query job object: {}", last_err());
-                return false;
-            }
-
-            let mut killed = false;
-            let list = &jobs.list[..jobs.header.NumberOfProcessIdsInList as usize];
-            assert!(!list.is_empty());
-            info!("found {} remaining processes", list.len() - 1);
-
-            let list = list.iter()
-                .filter(|&&id| {
-                    // let's not kill ourselves
-                    id as DWORD != GetCurrentProcessId()
-                })
-                .filter_map(|&id| {
-                    // Open the process with the necessary rights, and if this
-                    // fails then we probably raced with the process exiting so we
-                    // ignore the problem.
-                    let flags = PROCESS_QUERY_INFORMATION | PROCESS_TERMINATE | SYNCHRONIZE;
-                    let p = OpenProcess(flags, FALSE, id as DWORD);
-                    if p.is_null() {
-                        None
-                    } else {
-                        Some(Handle { inner: p })
-                    }
-                })
-                .filter(|p| {
-                    // Test if this process was actually in the job object or not.
-                    // If it's not then we likely raced with something else
-                    // recycling this PID, so we just skip this step.
-                    let mut res = 0;
-                    let r = IsProcessInJob(p.inner, self.job.inner, &mut res);
-                    if r == 0 {
-                        info!("failed to test is process in job: {}", last_err());
-                        return false;
-                    }
-                    res == TRUE
-                });
-
-            for p in list {
-                // Load the file which this process was spawned from. We then
-                // later use this for identification purposes.
-                let mut buf = [0; 1024];
-                let r = GetProcessImageFileNameW(p.inner, buf.as_mut_ptr(), buf.len() as DWORD);
-                if r == 0 {
-                    info!("failed to get image name: {}", last_err());
-                    continue;
-                }
-                let s = OsString::from_wide(&buf[..r as usize]);
-                info!("found remaining: {:?}", s);
-
-                // And here's where we find the whole purpose for this
-                // function!  Currently, our only whitelisted process is
-                // `mspdbsrv.exe`, and more details about that can be found
-                // here:
-                //
-                //      https://github.com/rust-lang/rust/issues/33145
-                //
-                // The gist of it is that all builds on one machine use the
-                // same `mspdbsrv.exe` instance. If we were to kill this
-                // instance then we could erroneously cause other builds to
-                // fail.
-                if let Some(s) = s.to_str() {
-                    if s.contains("mspdbsrv") {
-                        info!("\toops, this is mspdbsrv");
-                        continue;
-                    }
-                }
-
-                // Ok, this isn't mspdbsrv, let's kill the process. After we
-                // kill it we wait on it to ensure that the next time around in
-                // this function we're not going to see it again.
-                let r = TerminateProcess(p.inner, 1);
-                if r == 0 {
-                    info!("\tfailed to kill subprocess: {}", last_err());
-                    info!("\tassuming subprocess is dead...");
-                } else {
-                    info!("\tterminated subprocess");
-                }
-                let r = WaitForSingleObject(p.inner, INFINITE);
-                if r != 0 {
-                    info!("failed to wait for process to die: {}", last_err());
-                    return false;
-                }
-                killed = true;
-            }
-
-            killed
-        }
-    }
-
     impl Drop for Handle {
         fn drop(&mut self) {
             unsafe {