[blinkpy] Manage the lifecycle of wptserve better

During a recent roll of wpt tools, wptserve was broken and it was very
hard to debug. A few changes are made to improve the debuggability and
overall code health of wptserve management in blinkpy.

* Stop discarding the output of wptserve. Set the logging level to info
  (the default is debug) and preserve the output, so that it can be
  dumped out when something goes wrong.
* When checking and killing wptserve, we now poll the process before
  sending the null signal, as `kill -0` a defunct process will still
  succeed. We can now reap zombies much faster :)
* Lastly, _check_and_kill no longer does blocking wait inside. Now,
  _wait_for_action(_check_and_kill) makes more sense and matches the
  pattern in ServerBase better; and we send SIGKILL as a last resort
  (on POSIX).

apache_http.py is modified by the way to replace an unnecessary popen
with run_command. httpd exits immediately after it spawns the daemon,
so there's no use holding onto the defunct main process.

Change-Id: I441d36739451ad1e37afc6afe8f3c089cf224822
Reviewed-on: https://chromium-review.googlesource.com/1026889Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>
Commit-Queue: Robert Ma <robertma@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553619}

third_party/blink/tools/blinkpy/common/system/executive.py

@@ -111,8 +111,13 @@ class Executive(object):
 
         Will fail silently if pid does not exist or insufficient permissions.
         """
-        # According to http://docs.python.org/library/os.html
-        # os.kill isn't available on Windows.
+        # This method behaves differently on Windows and Linux. On Windows, it
+        # kills the process as well as all of its subprocesses (because of the
+        # '/t' flag). Some call sites depend on this behaviour (e.g. to kill all
+        # worker processes of wptserve on Windows).
+        # TODO(robertma): Replicate the behaviour on POSIX by calling setsid()
+        # in Popen's preexec_fn hook, and perhaps rename the method to
+        # kill_process_tree.
         if sys.platform == 'win32':
             command = ['taskkill.exe', '/f', '/t', '/pid', pid]
             # taskkill will exit 128 if the process is not found. We should log.

third_party/blink/tools/blinkpy/third_party/wpt/wpt.config.json

@@ -3,6 +3,7 @@
   "ws_doc_root": null,
   "check_subdomains": false,
   "server_host": "127.0.0.1",
+  "log_level": "info",
   "ports": {
     "http": [8001, 8081],
     "https": [8444],

third_party/blink/tools/blinkpy/web_tests/servers/apache_http.py

@@ -149,8 +149,8 @@ class ApacheHTTP(server_base.ServerBase):
 
     def _spawn_process(self):
         _log.debug('Starting %s server, cmd="%s"', self._name, str(self._start_cmd))
-        self._process = self._executive.popen(self._start_cmd)
-        retval = self._process.returncode
+        # Apache2 (or httpd) spawns a background daemon and immediately exits.
+        retval = self._executive.run_command(self._start_cmd, return_exit_code=True)
         if retval:
             raise server_base.ServerError('Failed to start %s: %s' % (self._name, retval))
 

third_party/blink/tools/blinkpy/web_tests/servers/wptserve.py

@@ -50,7 +50,6 @@ class WPTServe(server_base.ServerBase):
         if self._port_obj.host.filesystem.exists(path_to_ws_handlers):
             start_cmd += ['--ws_doc_root', path_to_ws_handlers]
 
-        self._stdout = self._stderr = self._executive.DEVNULL
         # TODO(burnik): We should stop setting the CWD once WPT can be run without it.
         self._cwd = path_to_wpt_root
         self._env = port_obj.host.environ.copy()
@@ -61,36 +60,41 @@ class WPTServe(server_base.ServerBase):
         if datetime.date.today() > expiration_date - datetime.timedelta(30):
             logging.getLogger(__name__).error(
                 'Pre-generated keys and certificates are going to be expired at %s.'
-                ' Please re-generate them by following steps in %s/README.chromium.'
-                % (expiration_date.strftime('%b %d %Y'), path_to_wpt_support))
+                ' Please re-generate them by following steps in %s/README.chromium.',
+                expiration_date.strftime('%b %d %Y'), path_to_wpt_support)
 
     def _stop_running_server(self):
-        self._wait_for_action(self._check_and_kill_wptserve)
+        if not self._wait_for_action(self._check_and_kill):
+            # This is mostly for POSIX systems. We send SIGINT in
+            # _check_and_kill() and here we use SIGKILL.
+            self._executive.kill_process(self._pid)
+
         if self._filesystem.exists(self._pid_file):
             self._filesystem.remove(self._pid_file)
 
-    def _check_and_kill_wptserve(self):
+    def _check_and_kill(self):
         """Tries to kill wptserve.
 
         Returns True if it appears to be not running. Or, if it appears to be
         running, tries to kill the process and returns False.
         """
-        if not (self._pid and self._executive.check_running_pid(self._pid)):
+        if not (self._pid and self._process):
+            _log.warning('No process object or PID. wptserve has not started.')
+            return True
+
+        # Polls the process in case it has died; otherwise, the process might be
+        # defunct and check_running_pid can still succeed.
+        if self._process.poll() or not self._executive.check_running_pid(self._pid):
             _log.debug('pid %d is not running', self._pid)
             return True
 
         _log.debug('pid %d is running, killing it', self._pid)
 
-        # Executive.kill_process appears to not to effectively kill the
-        # wptserve processes on Linux (and presumably other platforms).
+        # kill_process() kills the whole process tree on Windows, but not on
+        # POSIX, so we send SIGINT instead to allow wptserve to exit gracefully.
         if self._platform.is_win():
             self._executive.kill_process(self._pid)
         else:
             self._executive.interrupt(self._pid)
 
-        # According to Popen.wait(), this can deadlock when using stdout=PIPE or
-        # stderr=PIPE. We're using DEVNULL for both so that should not occur.
-        if self._process is not None:
-            self._process.wait()
-
         return False

third_party/blink/tools/blinkpy/web_tests/servers/wptserve_unittest.py

@@ -4,8 +4,9 @@
 
 import logging
 
-from blinkpy.common.system.log_testing import LoggingTestCase
 from blinkpy.common.host_mock import MockHost
+from blinkpy.common.system.executive_mock import MockProcess
+from blinkpy.common.system.log_testing import LoggingTestCase
 from blinkpy.web_tests.port import test
 from blinkpy.web_tests.servers.wptserve import WPTServe
 
@@ -54,6 +55,7 @@ class TestWPTServe(LoggingTestCase):
         server = WPTServe(test_port, '/log_file_dir')
         server._pid_file = '/tmp/pidfile'
         server._spawn_process = lambda: 4
+        server._process = MockProcess()
         server._is_server_running_on_all_ports = lambda: True
 
         # Simulate a process that never gets killed.