Fix infinite hang in wait_for_process_to_die() on Windows

CHANGES.txt

@@ -214,6 +214,10 @@ RELEASE  VERSION/DATE TO BE FILLED IN LATER
       `local_pool`, hoping it will fix a race condition that can occurs when
       Ninja defers to SCons to build.
 
+    From Adam Simpkins:
+    - Fixed a hang in `wait_for_process_to_die()` on Windows, affecting
+      clean-up of the SCons daemon used for Ninja builds.
+
 
 RELEASE 4.8.1 -  Tue, 03 Sep 2024 17:22:20 -0700
 

RELEASE.txt

@@ -183,6 +183,9 @@ FIXES
 - Handle case of "memoizer" as one member of a comma-separated
   --debug string - this was previously missed.
 
+- Fixed a hang in `wait_for_process_to_die()` on Windows, affecting
+  clean-up of the SCons daemon used for Ninja builds.
+
 IMPROVEMENTS
 ------------
 

SCons/Util/UtilTests.py

@@ -27,11 +27,13 @@
 import hashlib
 import io
 import os
+import subprocess
 import sys
 import unittest
 import unittest.mock
 import warnings
 from collections import UserDict, UserList, UserString, namedtuple
+from typing import Callable
 
 import TestCmd
 
@@ -73,6 +75,8 @@
     to_String,
     to_bytes,
     to_str,
+    wait_for_process_to_die,
+    _wait_for_process_to_die_non_psutil,
 )
 from SCons.Util.envs import is_valid_construction_var
 from SCons.Util.hashes import (
@@ -82,6 +86,13 @@
     _set_allowed_viable_default_hashes,
 )
 
+try:
+    import psutil
+    has_psutil = True
+except ImportError:
+    has_psutil = False
+
+
 # These Util classes have no unit tests. Some don't make sense to test?
 # DisplayEngine, Delegate, MethodWrapper, UniqueList, Unbuffered, Null, NullSeq
 
@@ -847,6 +858,21 @@ def test_intern(self) -> None:
         s4 = silent_intern("spam")
         assert id(s1) == id(s4)
 
+    @unittest.skipUnless(has_psutil, "requires psutil")
+    def test_wait_for_process_to_die_success_psutil(self) -> None:
+        self._test_wait_for_process(wait_for_process_to_die)
+
+    def test_wait_for_process_to_die_success_non_psutil(self) -> None:
+        self._test_wait_for_process(_wait_for_process_to_die_non_psutil)
+
+    def _test_wait_for_process(
+        self, wait_fn: Callable[[int], None]
+    ) -> None:
+        cmd = [sys.executable, "-c", ""]
+        p = subprocess.Popen(cmd)
+        p.wait()
+        wait_fn(p.pid)
+
 
 class HashTestCase(unittest.TestCase):
 

SCons/Util/__init__.py

@@ -1297,36 +1297,70 @@ def print_time():
     return print_time
 
 
-def wait_for_process_to_die(pid) -> None:
+def wait_for_process_to_die(pid: int) -> None:
     """
-    Wait for specified process to die, or alternatively kill it
-    NOTE: This function operates best with psutil pypi package
+    Wait for the specified process to die.
+
     TODO: Add timeout which raises exception
     """
     # wait for the process to fully killed
     try:
         import psutil  # pylint: disable=import-outside-toplevel
         while True:
+            # TODO: this should use psutil.process_exists() or psutil.Process.wait()
+            # The psutil docs explicitly recommend against using process_iter()/pids()
+            # for checking the existence of a process.
             if pid not in [proc.pid for proc in psutil.process_iter()]:
                 break
             time.sleep(0.1)
     except ImportError:
         # if psutil is not installed we can do this the hard way
-        while True:
-            if sys.platform == 'win32':
-                import ctypes  # pylint: disable=import-outside-toplevel
-                PROCESS_QUERY_INFORMATION = 0x1000
-                processHandle = ctypes.windll.kernel32.OpenProcess(PROCESS_QUERY_INFORMATION, 0, pid)
-                if processHandle == 0:
-                    break
-                ctypes.windll.kernel32.CloseHandle(processHandle)
-                time.sleep(0.1)
-            else:
-                try:
-                    os.kill(pid, 0)
-                except OSError:
-                    break
-                time.sleep(0.1)
+        _wait_for_process_to_die_non_psutil(pid, timeout=-1.0)
+
+
+def _wait_for_process_to_die_non_psutil(pid: int, timeout: float = 60.0) -> None:
+    start_time = time.time()
+    while True:
+        if not _is_process_alive(pid):
+            break
+        if timeout >= 0.0 and time.time() - start_time > timeout:
+            raise TimeoutError(f"timed out waiting for process {pid}")
+        time.sleep(0.1)
+
+
+if sys.platform == 'win32':
+    def _is_process_alive(pid: int) -> bool:
+        import ctypes  # pylint: disable=import-outside-toplevel
+        PROCESS_QUERY_INFORMATION = 0x1000
+        STILL_ACTIVE = 259
+
+        processHandle = ctypes.windll.kernel32.OpenProcess(PROCESS_QUERY_INFORMATION, 0, pid)
+        if processHandle == 0:
+            return False
+
+        # OpenProcess() may successfully return a handle even for terminated
+        # processes when something else in the system is still holding a
+        # reference to their handle.  Call GetExitCodeProcess() to check if the
+        # process has already exited.
+        try:
+            exit_code = ctypes.c_ulong()
+            success = ctypes.windll.kernel32.GetExitCodeProcess(
+                    processHandle, ctypes.byref(exit_code))
+            if success:
+                return exit_code.value == STILL_ACTIVE
+        finally:
+            ctypes.windll.kernel32.CloseHandle(processHandle)
+
+        return True
+
+else:
+    def _is_process_alive(pid: int) -> bool:
+        try:
+            os.kill(pid, 0)
+            return True
+        except OSError:
+            return False
+
 
 # From: https://stackoverflow.com/questions/1741972/how-to-use-different-formatters-with-the-same-logging-handler-in-python
 class DispatchingFormatter(Formatter):