Bug 1433905 - [mozprocess] poll() always returns None for stopped process until wait() is called. r=gbrown
authorHenrik Skupin <mail@hskupin.info>
Thu, 04 Oct 2018 14:51:32 +0000
changeset 495371 21e66bfa0cc4947247ba659e70e586c96c798792
parent 495370 8793e332890e9dab0c931d92e295965028ad4bef
child 495372 0a653b330b03ec8ce30b31275fa083061d698cc4
push id9984
push userffxbld-merge
push dateMon, 15 Oct 2018 21:07:35 +0000
treeherdermozilla-beta@183d27ea8570 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgbrown
bugs1433905
milestone64.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1433905 - [mozprocess] poll() always returns None for stopped process until wait() is called. r=gbrown If the process quits externally (shutdown by itself or via kill), the poll method still returns None, even with the process not existent anymore. To fix that, the poll method should at least try to join the reader thread once before checking if it is still alive. Otherwise it will continue to run, and never stop. Also the attribute existence check for "returncode" on the process instance has to be removed because this property always exists. Instead a check for the "returncode" property of the ProcessHandler class is necessary. Depends on D7396 Differential Revision: https://phabricator.services.mozilla.com/D7398
testing/mozbase/mozprocess/mozprocess/processhandler.py
testing/mozbase/mozprocess/tests/test_poll.py
testing/mozbase/mozprocess/tests/test_wait.py
--- a/testing/mozbase/mozprocess/mozprocess/processhandler.py
+++ b/testing/mozbase/mozprocess/mozprocess/processhandler.py
@@ -18,16 +18,19 @@ from datetime import datetime
 
 
 __all__ = ['ProcessHandlerMixin', 'ProcessHandler', 'LogOutput',
            'StoreOutput', 'StreamOutput']
 
 # Set the MOZPROCESS_DEBUG environment variable to 1 to see some debugging output
 MOZPROCESS_DEBUG = os.getenv("MOZPROCESS_DEBUG")
 
+
+INTERVAL_PROCESS_ALIVE_CHECK = 0.02
+
 # We dont use mozinfo because it is expensive to import, see bug 933558.
 isWin = os.name == "nt"
 isPosix = os.name == "posix"  # includes MacOS X
 
 if isWin:
     from ctypes import sizeof, addressof, c_ulong, byref, WinError, c_longlong
     from . import winprocess
     from .qijo import JobObjectAssociateCompletionPortInformation,\
@@ -184,17 +187,17 @@ class ProcessHandlerMixin(object):
                 if sig is None and isPosix:
                     # ask the process for termination and wait a bit
                     send_sig(signal.SIGTERM)
                     limit = time.time() + self.TIMEOUT_BEFORE_SIGKILL
                     while time.time() <= limit:
                         if self.poll() is not None:
                             # process terminated nicely
                             break
-                        time.sleep(0.02)
+                        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()
@@ -816,26 +819,26 @@ falling back to not using job objects fo
         """Check if child process has terminated
 
         Returns the current returncode value:
         - None if the process hasn't terminated yet
         - A negative number if the process was killed by signal N (Unix only)
         - '0' if the process ended without failures
 
         """
+        if not hasattr(self, "proc"):
+            raise RuntimeError("Process hasn't been started yet")
+
         # Ensure that we first check for the reader status. Otherwise
         # we might mark the process as finished while output is still getting
         # processed.
-        if not hasattr(self, "proc"):
-            raise RuntimeError("Process hasn't been started yet")
-
         elif self.reader.is_alive():
             return None
-        elif hasattr(self.proc, "returncode"):
-            return self.proc.returncode
+        elif hasattr(self, "returncode"):
+            return self.returncode
         else:
             return self.proc.poll()
 
     def processOutput(self, timeout=None, outputTimeout=None):
         """
         Handle process output until the process terminates or times out.
 
         If timeout is not None, the process will be allowed to continue for
@@ -865,22 +868,22 @@ falling back to not using job objects fo
         does not kill the process.
 
         Returns the process exit code value:
         - None if the process hasn't terminated yet
         - A negative number if the process was killed by signal N (Unix only)
         - '0' if the process ended without failures
 
         """
+        # Thread.join() blocks the main thread until the reader thread is finished
+        # wake up once a second in case a keyboard interrupt is sent
         if self.reader.thread and self.reader.thread is not threading.current_thread():
-            # Thread.join() blocks the main thread until the reader thread is finished
-            # wake up once a second in case a keyboard interrupt is sent
             count = 0
             while self.reader.is_alive():
-                self.reader.thread.join(timeout=1)
+                self.reader.join(timeout=1)
                 count += 1
                 if timeout is not None and count > timeout:
                     return None
 
         self.returncode = self.proc.wait()
         return self.returncode
 
     @property
@@ -1025,17 +1028,17 @@ class ProcessReader(object):
         output_timeout = self.output_timeout
         if output_timeout is not None:
             output_timeout += start_time
 
         while (stdout_reader and stdout_reader.is_alive()) \
                 or (stderr_reader and stderr_reader.is_alive()):
             has_line = True
             try:
-                line, callback = queue.get(True, 0.02)
+                line, callback = queue.get(True, INTERVAL_PROCESS_ALIVE_CHECK)
             except Empty:
                 has_line = False
             now = time.time()
             if not has_line:
                 if output_timeout is not None and now > output_timeout:
                     timed_out = True
                     self.didOutputTimeout = True
                     break
@@ -1059,16 +1062,21 @@ class ProcessReader(object):
         if not timed_out:
             self.finished_callback()
 
     def is_alive(self):
         if self.thread:
             return self.thread.is_alive()
         return False
 
+    def join(self, timeout=None):
+        if self.thread:
+            self.thread.join(timeout=timeout)
+
+
 # default output handlers
 # these should be callables that take the output line
 
 
 class StoreOutput(object):
     """accumulate stdout"""
 
     def __init__(self):
--- a/testing/mozbase/mozprocess/tests/test_poll.py
+++ b/testing/mozbase/mozprocess/tests/test_poll.py
@@ -1,14 +1,15 @@
 #!/usr/bin/env python
 
 from __future__ import absolute_import
 
 import os
 import signal
+import time
 
 import mozinfo
 import mozunit
 
 from mozprocess import processhandler
 
 import proctest
 
@@ -103,26 +104,33 @@ class ProcTestPoll(proctest.ProcTest):
         self.determine_status(p)
 
     def test_poll_after_external_kill(self):
         """Process is killed externally, and poll() is called."""
         p = processhandler.ProcessHandler([self.python, self.proclaunch,
                                            "process_normal_finish.ini"],
                                           cwd=here)
         p.run()
+
         os.kill(p.pid, signal.SIGTERM)
-        returncode = p.wait()
+
+        # Allow the output reader thread to finish processing remaining data
+        for i in xrange(0, 100):
+            time.sleep(processhandler.INTERVAL_PROCESS_ALIVE_CHECK)
+            returncode = p.poll()
+            if returncode is not None:
+                break
 
         # We killed the process, so the returncode should be non-zero
         if mozinfo.isWin:
             self.assertEqual(returncode, signal.SIGTERM,
                              'Positive returncode expected, got "%s"' % returncode)
         else:
             self.assertEqual(returncode, -signal.SIGTERM,
                              '%s expected, got "%s"' % (-signal.SIGTERM, returncode))
 
-        self.assertEqual(returncode, p.poll())
+        self.assertEqual(returncode, p.wait())
 
         self.determine_status(p)
 
 
 if __name__ == '__main__':
     mozunit.main()
--- a/testing/mozbase/mozprocess/tests/test_wait.py
+++ b/testing/mozbase/mozprocess/tests/test_wait.py
@@ -1,20 +1,22 @@
 #!/usr/bin/env python
 
 from __future__ import absolute_import
 
 import os
-import proctest
+import signal
+
 import mozinfo
-
 import mozunit
 
 from mozprocess import processhandler
 
+import proctest
+
 here = os.path.dirname(os.path.abspath(__file__))
 
 
 class ProcTestWait(proctest.ProcTest):
     """ Class to test process waits and timeouts """
 
     def test_normal_finish(self):
         """Process is started, runs to completion while we wait for it"""
@@ -96,11 +98,32 @@ class ProcTestWait(proctest.ProcTest):
             self.assertGreater(returncode2, 0,
                                'Positive returncode expected, got "%s"' % returncode2)
         else:
             self.assertLess(returncode2, 0,
                             'Negative returncode expected, got "%s"' % returncode2)
         self.assertEqual(returncode1, returncode2,
                          'Expected both returncodes of wait() to be equal')
 
+    def test_wait_after_external_kill(self):
+        """Process is killed externally, and poll() is called."""
+        p = processhandler.ProcessHandler([self.python, self.proclaunch,
+                                           "process_normal_finish.ini"],
+                                          cwd=here)
+        p.run()
+        os.kill(p.pid, signal.SIGTERM)
+        returncode = p.wait()
+
+        # We killed the process, so the returncode should be non-zero
+        if mozinfo.isWin:
+            self.assertEqual(returncode, signal.SIGTERM,
+                             'Positive returncode expected, got "%s"' % returncode)
+        else:
+            self.assertEqual(returncode, -signal.SIGTERM,
+                             '%s expected, got "%s"' % (-signal.SIGTERM, returncode))
+
+        self.assertEqual(returncode, p.poll())
+
+        self.determine_status(p)
+
 
 if __name__ == '__main__':
     mozunit.main()