Bug 1705385 - Allow passing a timeout to mozprocess.ProcessHandler.kill, r=ahal draft
authorJames Graham <james@hoppipolla.co.uk>
Mon, 19 Apr 2021 21:12:03 +0000
changeset 3668286 d234160b358d40c723578a3ae5d996e3d7493ec1
parent 3668285 87ab9f0a7ce9893429074cbb1c8e4e6c9e60398e
child 3668287 e595080f8534b57cd4fda3d0975abf7ec1b1af3f
push id683208
push userreviewbot
push dateMon, 19 Apr 2021 21:12:39 +0000
treeherdertry@e595080f8534 [default view] [failures only]
reviewersahal
bugs1705385
milestone89.0a1
Bug 1705385 - Allow passing a timeout to mozprocess.ProcessHandler.kill, r=ahal Unlike the stdlib Popen class, ProcessHandler.kill implictly waits for the process to end. To avoid hangs in the case where the process doesn't end, allow passing through a timeout to this function (on posix only). This is somewhat unfortunate as it means that ProcessHandler isn't interchangable with Popen. But the only other option here appears to be not doing the implicit wait which presumably consumers are relying on. Differential Diff: PHID-DIFF-vs3bspsg4xzpo7am3vz4
testing/mozbase/mozprocess/mozprocess/processhandler.py
testing/mozbase/mozprocess/tests/test_kill.py
--- a/testing/mozbase/mozprocess/mozprocess/processhandler.py
+++ b/testing/mozbase/mozprocess/mozprocess/processhandler.py
@@ -188,17 +188,17 @@ class ProcessHandlerMixin(object):
                 handle = getattr(self, "_handle", None)
                 if handle:
                     self._internal_poll(_deadstate=_maxint)
                 if handle or self._job or self._io_port:
                     self._cleanup()
             else:
                 subprocess.Popen.__del__(self)
 
-        def kill(self, sig=None):
+        def kill(self, sig=None, timeout=None):
             if isWin:
                 try:
                     if not self._ignore_children and self._handle and self._job:
                         self.debug("calling TerminateJobObject")
                         winprocess.TerminateJobObject(
                             self._job, winprocess.ERROR_CONTROL_C_EXIT
                         )
                     elif self._handle:
@@ -255,17 +255,17 @@ class ProcessHandlerMixin(object):
                         time.sleep(INTERVAL_PROCESS_ALIVE_CHECK)
                     else:
                         # process did not terminate - send SIGKILL to force
                         send_sig(signal.SIGKILL)
                 else:
                     # a signal was explicitly set or not posix
                     send_sig(sig or signal.SIGKILL)
 
-            self.returncode = self.wait()
+            self.returncode = self.wait(timeout)
             self._cleanup()
             return self.returncode
 
         def poll(self):
             """Popen.poll
             Check if child process has terminated. Set and return returncode attribute.
             """
             # If we have a handle, the process is alive
@@ -947,17 +947,17 @@ falling back to not using job objects fo
 
         if isPosix:
             # Keep track of the initial process group in case the process detaches itself
             self.proc.pgid = self._getpgid(self.proc.pid)
             self.proc.detached_pid = None
 
         self.processOutput(timeout=timeout, outputTimeout=outputTimeout)
 
-    def kill(self, sig=None):
+    def kill(self, sig=None, timeout=None):
         """
         Kills the managed process.
 
         If you created the process with 'ignore_children=False' (the
         default) then it will also also kill all child processes spawned by
         it. If you specified 'ignore_children=True' when creating the
         process, only the root process will be killed.
 
@@ -965,22 +965,22 @@ falling back to not using job objects fo
         it immediately kills the process.
 
         :param sig: Signal used to kill the process, defaults to SIGKILL
                     (has no effect on Windows)
         """
         if not hasattr(self, "proc"):
             raise RuntimeError("Process hasn't been started yet")
 
-        self.proc.kill(sig=sig)
+        self.proc.kill(sig=sig, timeout=timeout)
 
         # When we kill the the managed process we also have to wait for the
         # reader thread to be finished. Otherwise consumers would have to assume
         # that it still has not completely shutdown.
-        rc = self.wait()
+        rc = self.wait(timeout)
         if rc is None:
             self.debug("kill: wait failed -- process is still alive")
         return rc
 
     def poll(self):
         """Check if child process has terminated
 
         Returns the current returncode value:
@@ -1044,17 +1044,17 @@ falling back to not using job objects fo
             count = 0
             while self.reader.is_alive():
                 self.reader.join(timeout=1)
                 count += 1
                 if timeout is not None and count > timeout:
                     self.debug("wait timeout for reader thread")
                     return None
 
-        self.returncode = self.proc.wait()
+        self.returncode = self.proc.wait(timeout)
         return self.returncode
 
     @property
     def pid(self):
         if not hasattr(self, "proc"):
             raise RuntimeError("Process hasn't been started yet")
 
         return self.proc.pid
--- a/testing/mozbase/mozprocess/tests/test_kill.py
+++ b/testing/mozbase/mozprocess/tests/test_kill.py
@@ -122,11 +122,26 @@ class ProcTestKill(proctest.ProcTest):
         p = processhandler.ProcessHandler([self.python, script, "deadlock"])
 
         p.run()
         time.sleep(1)
         p.kill()
 
         self.assertEquals(p.proc.returncode, -signal.SIGKILL)
 
+    @unittest.skipUnless(processhandler.isPosix, "posix only")
+    def test_process_kill_with_timeout(self):
+        script = os.path.join(here, "scripts", "ignore_sigterm.py")
+        p = processhandler.ProcessHandler([self.python, script])
+
+        p.run()
+        time.sleep(1)
+        t0 = time.time()
+        p.kill(sig=signal.SIGTERM, timeout=2)
+        self.assertEquals(p.proc.returncode, None)
+        self.assertGreaterEqual(time.time(), t0 + 2)
+
+        p.kill(sig=signal.SIGKILL)
+        self.assertEquals(p.proc.returncode, -signal.SIGKILL)
+
 
 if __name__ == "__main__":
     mozunit.main()