Rollup merge of rust-lang#133888 - ChrisDenton:job, r=jieyouxu

Improve bootstrap job objects

This attempts to fix a few comments on bootstrap job objects.

I also fixed an issue where if duplicating the job object handle into the python process failed, it would close the job object. This would then result in the job object closing all attached processes, which at that point includes the current process. The fix is to simply never close the job object handle at any point after the current process is assigned to it.

Author: jhpratt

src/bootstrap/bootstrap.py

@@ -1184,6 +1184,8 @@ def bootstrap(args):
     args = [build.bootstrap_binary()]
     args.extend(sys.argv[1:])
     env = os.environ.copy()
+    # The Python process ID is used when creating a Windows job object
+    # (see src\bootstrap\src\utils\job.rs)
     env["BOOTSTRAP_PARENT_ID"] = str(os.getpid())
     env["BOOTSTRAP_PYTHON"] = sys.executable
     run(args, env=env, verbose=build.verbose, is_bootstrap=True)

src/bootstrap/src/utils/job.rs

@@ -15,11 +15,12 @@ pub unsafe fn setup(build: &mut crate::Build) {
 ///
 /// Most of the time when you're running a build system (e.g., make) you expect
 /// Ctrl-C or abnormal termination to actually terminate the entire tree of
-/// process in play, not just the one at the top. This currently works "by
+/// processes in play. 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 the
-/// parent process.
+/// *process group* so everything will get torn
+/// down. On Windows, however, Ctrl-C is only sent to processes in the same console.
+/// If a process is detached or attached to another console, it won't receive the
+/// signal.
 ///
 /// 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
@@ -87,15 +88,7 @@ mod for_windows {
         );
         assert!(r.is_ok(), "{}", io::Error::last_os_error());
 
-        // Assign our process to this job object. Note that if this fails, one very
-        // likely reason is that we are ourselves already in a job object! This can
-        // happen on the build bots that we've got for Windows, or if just anyone
-        // else is instrumenting the build. In this case we just bail out
-        // immediately and assume that they take care of it.
-        //
-        // Also note that nested jobs (why this might fail) are supported in recent
-        // versions of Windows, but the version of Windows that our bots are running
-        // at least don't support nested job objects.
+        // Assign our process to this job object.
         let r = AssignProcessToJobObject(job, GetCurrentProcess());
         if r.is_err() {
             CloseHandle(job).ok();
@@ -124,14 +117,19 @@ mod for_windows {
                 // (only when wrongly setting the environmental variable),
                 // it might be better to improve the experience of the second case
                 // when users have interrupted the parent process and we haven't finish
-                // duplicating the handle yet. We just need close the job object if that occurs.
-                CloseHandle(job).ok();
+                // duplicating the handle yet.
                 return;
             }
         };
 
         let mut parent_handle = HANDLE::default();
-        let r = DuplicateHandle(
+        // If this fails, well at least we tried! An example of DuplicateHandle
+        // failing in the past has been when the wrong python2 package spawned this
+        // build system (e.g., the `python2` package in MSYS instead of
+        // `mingw-w64-x86_64-python2`). Not sure why it failed, but the "failure
+        // mode" here is that we only clean everything up when the build system
+        // dies, not when the python parent does, so not too bad.
+        let _ = DuplicateHandle(
             GetCurrentProcess(),
             job,
             parent,
@@ -140,15 +138,6 @@ mod for_windows {
             false,
             DUPLICATE_SAME_ACCESS,
         );
-
-        // If this failed, well at least we tried! An example of DuplicateHandle
-        // failing in the past has been when the wrong python2 package spawned this
-        // build system (e.g., the `python2` package in MSYS instead of
-        // `mingw-w64-x86_64-python2`). Not sure why it failed, but the "failure
-        // mode" here is that we only clean everything up when the build system
-        // dies, not when the python parent does, so not too bad.
-        if r.is_err() {
-            CloseHandle(job).ok();
-        }
+        CloseHandle(parent).ok();
     }
 }