Apply review feedback

Author: simpkins

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/Tool/ninja_tool/NinjaState.py

@@ -694,8 +694,9 @@ def check_generated_source_deps(build):
                         pass
 
                 # wait for the server process to fully killed
-                # TODO: catch TimeoutException here and possibly do something smart.
-                wait_for_process_to_die(pid, timeout=10)
+                # TODO: update wait_for_process_to_die() to handle timeout and then catch exception
+                #       here and do something smart.
+                wait_for_process_to_die(pid)
 
             if os.path.exists(scons_daemon_dirty):
                 os.unlink(scons_daemon_dirty)

SCons/Util/UtilTests.py

@@ -33,6 +33,7 @@
 import unittest.mock
 import warnings
 from collections import UserDict, UserList, UserString, namedtuple
+from typing import Callable
 
 import TestCmd
 
@@ -75,6 +76,7 @@
     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 (
@@ -84,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
 
@@ -849,26 +858,20 @@ def test_intern(self) -> None:
         s4 = silent_intern("spam")
         assert id(s1) == id(s4)
 
-    def test_wait_for_process_to_die_success(self) -> None:
-        cmd = [sys.executable, "-c", ""]
-        p = subprocess.Popen(cmd)
-        p.wait()
-        wait_for_process_to_die(p.pid, timeout=10.0)
+    @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_timeout(self) -> None:
-        # Run a python script that will keep running until we close it's stdin
-        cmd = [sys.executable, "-c", "import sys; data = sys.stdin.read()"]
-        p = subprocess.Popen(cmd, stdin=subprocess.PIPE)
+    def test_wait_for_process_to_die_success_non_psutil(self) -> None:
+        self._test_wait_for_process(_wait_for_process_to_die_non_psutil)
 
-        # wait_for_process_to_die() should time out while the process is running
-        with self.assertRaises(TimeoutError):
-            wait_for_process_to_die(p.pid, timeout=0.2)
-
-        p.stdin.close()
+    def _test_wait_for_process(
+        self, wait_fn: Callable[[int], None]
+    ) -> None:
+        cmd = [sys.executable, "-c", ""]
+        p = subprocess.Popen(cmd)
         p.wait()
-
-        # wait_for_process_to_die() should complete normally now
-        wait_for_process_to_die(p.pid, timeout=10.0)
+        wait_fn(p.pid)
 
 
 class HashTestCase(unittest.TestCase):

SCons/Util/__init__.py

@@ -1297,14 +1297,28 @@ def print_time():
     return print_time
 
 
-def wait_for_process_to_die(pid: int, timeout: float = 60.0) -> None:
+def wait_for_process_to_die(pid: int) -> None:
     """
     Wait for the specified process to die.
 
-    A TimeoutError will be thrown if the timeout is hit before the process terminates.
-    Specifying a negative timeout will cause wait_for_process_to_die() to wait
-    indefinitely, and never throw TimeoutError.
+    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
+        _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):