Bug 1705385 - Allow passing a timeout to mozprocess.ProcessHandler.kill, r=ahal draft
authorJames Graham <james@hoppipolla.co.uk>
Mon, 19 Apr 2021 21:13:16 +0000
changeset 3668289 0f8b8b08542e11a8e23a099eadf6c9a51ce044a7
parent 3668288 2774ee1ead74302c72d71f095491b4e1330bf8fd
child 3668290 9567b424bcea2d0996fd5c444c2365890bdf1d7d
push id683209
push userreviewbot
push dateMon, 19 Apr 2021 21:13:46 +0000
treeherdertry@3fb65f6925ce [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()