[blinkpy] Poll server for aliveness in cli_wrapper

This prevents the server from dying silently in run_blink_[SERVER].py
by checking whether the server is alive every second. (See the race
condition where the server dies after a seemingly successful start in
the linked issue.)

Unfortunately, this does not fix run_web_tests.py.

Bug: 1090491
Change-Id: Ifb5a73284716a980a752ce3ed9c8210ba9510f8e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2229472
Commit-Queue: Robert Ma <robertma@chromium.org>
Reviewed-by: Stephen McGruer <smcgruer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#775251}

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

@@ -38,9 +38,10 @@ import signal
 
 from blinkpy.common.host import Host
 from blinkpy.common.system.log_utils import configure_logging
+from blinkpy.web_tests.port.base import ARTIFACTS_SUB_DIR
 from blinkpy.web_tests.port.factory import configuration_options
 from blinkpy.web_tests.port.factory import python_server_options
-from blinkpy.web_tests.port.base import ARTIFACTS_SUB_DIR
+from blinkpy.web_tests.servers.server_base import ServerError
 
 _log = logging.getLogger(__name__)
 
@@ -64,8 +65,7 @@ def main(server_constructor,
          description=None,
          **kwargs):
     host = Host()
-    # Signals will interrupt sleep, so we can use a long duration.
-    sleep_fn = sleep_fn or (lambda: host.sleep(10))
+    sleep_fn = sleep_fn or (lambda: host.sleep(1))
 
     parser = optparse.OptionParser(
         description=description, formatter=RawTextHelpFormatter())
@@ -106,6 +106,11 @@ def main(server_constructor,
     try:
         while True:
             sleep_fn()
+            if not server.alive():
+                raise ServerError('Server is no longer listening')
+    except ServerError as e:
+        _log.error(e)
     except (SystemExit, KeyboardInterrupt):
         _log.info('Exiting...')
+    finally:
         server.stop()

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

@@ -13,6 +13,7 @@ class MockServer(object):
         self.kwargs = kwargs
         self.start_called = False
         self.stop_called = False
+        self.is_alive = True
 
     def start(self):
         self.start_called = True
@@ -20,12 +21,15 @@ class MockServer(object):
     def stop(self):
         self.stop_called = True
 
+    def alive(self):
+        return self.is_alive
+
 
 class CliWrapperTest(unittest.TestCase):
     def setUp(self):
         self.server = None
 
-    def test_main(self):
+    def test_main_success(self):
         def mock_server_constructor(*args, **kwargs):
             self.server = MockServer(args, kwargs)
             return self.server
@@ -36,3 +40,17 @@ class CliWrapperTest(unittest.TestCase):
         cli_wrapper.main(mock_server_constructor, sleep_fn=raise_exit, argv=[])
         self.assertTrue(self.server.start_called)
         self.assertTrue(self.server.stop_called)
+
+    def test_main_server_error_after_start(self):
+        def mock_server_constructor(*args, **kwargs):
+            self.server = MockServer(args, kwargs)
+            return self.server
+
+        def server_error():
+            self.server.is_alive = False
+
+        cli_wrapper.main(mock_server_constructor,
+                         sleep_fn=server_error,
+                         argv=[])
+        self.assertTrue(self.server.start_called)
+        self.assertTrue(self.server.stop_called)

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

@@ -75,7 +75,11 @@ class ServerBase(object):
         # redirect them to files.
         self._stdout = self._executive.PIPE
         self._stderr = self._executive.PIPE
+        # The entrypoint process of the server, which may not be the daemon,
+        # e.g. apachectl.
         self._process = None
+        # The PID of the server daemon, which may be different from
+        # self._process.pid.
         self._pid = None
         self._error_log_path = None
 
@@ -150,6 +154,14 @@ class ServerBase(object):
             # Make sure we delete the pid file no matter what happens.
             self._remove_pid_file()
 
+    def alive(self):
+        """Checks whether the server is alive."""
+        # This by default checks both the process and ports.
+        # At this point, we think the server has started up, so successes are
+        # normal while failures are not.
+        return self._is_server_running_on_all_ports(
+            success_log_level=logging.DEBUG, failure_log_level=logging.INFO)
+
     def _prepare_config(self):
         """This routine can be overridden by subclasses to do any sort
         of initialization required prior to starting the server that may fail.
@@ -252,9 +264,17 @@ class ServerBase(object):
 
         return False
 
-    def _is_server_running_on_all_ports(self):
-        """Returns whether the server is running on all the desired ports."""
+    def _is_server_running_on_all_ports(self,
+                                        success_log_level=logging.INFO,
+                                        failure_log_level=logging.DEBUG):
+        """Returns whether the server is running on all the desired ports.
 
+        Args:
+            success_log_level: Logging level for success (default: INFO)
+            failure_log_level: Logging level for failure (default: DEBUG)
+        """
+        # Check self._pid instead of self._process because the latter might be a
+        # control process that exits after spawning up the daemon.
         # TODO(dpranke): crbug/378444 maybe pid is unreliable on win?
         if (not self._platform.is_win()
                 and not self._executive.check_running_pid(self._pid)):
@@ -268,12 +288,14 @@ class ServerBase(object):
             scheme = mapping['scheme']
             try:
                 s.connect(('localhost', port))
-                _log.info('Server running on %s://localhost:%d', scheme, port)
+                _log.log(success_log_level,
+                         'Server running on %s://localhost:%d', scheme, port)
             except IOError as error:
                 if error.errno not in (errno.ECONNREFUSED, errno.ECONNRESET):
                     raise
-                _log.debug('Server NOT running on %s://localhost:%d : %s',
-                           scheme, port, error)
+                _log.log(failure_log_level,
+                         'Server NOT running on %s://localhost:%d : %s',
+                         scheme, port, error)
                 return False
             finally:
                 s.close()