Bug 1290372 - _wait_for_connection() has to return early if instance is not running. r=ato
☠☠ backed out by a7fc516956bc ☠ ☠
authorHenrik Skupin <mail@hskupin.info>
Mon, 17 Oct 2016 11:51:40 +0200
changeset 318202 198c4bf0c8df9765e9eba5f12eb1f7a899c277e5
parent 318201 e4ef6fa03aa8689479827c4c7ea6dd5d26675c01
child 318203 cf68c36df030b2ed0f0dd65199a681e7b6fef74f
push id33212
push userhskupin@mozilla.com
push dateMon, 17 Oct 2016 09:55:21 +0000
treeherderautoland@198c4bf0c8df [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersato
bugs1290372
milestone52.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 1290372 - _wait_for_connection() has to return early if instance is not running. r=ato In case when the instance is not running, the method would check for the port again and again until the timeout is reached. This extra time can be spent if the process status is checked. MozReview-Commit-ID: C2WAWNC5CWE
testing/marionette/client/marionette_driver/marionette.py
testing/marionette/client/marionette_driver/transport.py
testing/marionette/harness/marionette/tests/unit/test_marionette.py
--- a/testing/marionette/client/marionette_driver/marionette.py
+++ b/testing/marionette/client/marionette_driver/marionette.py
@@ -1,18 +1,20 @@
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
+import ConfigParser
 import base64
-import ConfigParser
+import datetime
 import json
 import os
 import socket
 import sys
+import time
 import traceback
 import warnings
 
 from contextlib import contextmanager
 
 from decorators import do_process_check
 from keys import Keys
 
@@ -566,21 +568,20 @@ class Marionette(object):
         self.session_id = None
         self.window = None
         self.chrome_window = None
         self.baseurl = baseurl
         self._test_name = None
         self.timeout = timeout
         self.socket_timeout = socket_timeout
 
-        startup_timeout = startup_timeout or self.DEFAULT_STARTUP_TIMEOUT
         if self.bin:
             self.instance = self._create_instance(app, instance_args)
             self.instance.start()
-            self.raise_for_port(self.wait_for_port(timeout=startup_timeout))
+            self.raise_for_connection(timeout=startup_timeout)
 
     def _create_instance(self, app, instance_args):
         if not Marionette.is_port_available(self.port, host=self.host):
             ex_msg = "{0}:{1} is unavailable.".format(self.host, self.port)
             raise errors.MarionetteException(message=ex_msg)
         if app:
             # select instance class for the given app
             try:
@@ -634,24 +635,65 @@ class Marionette(object):
         try:
             s.bind((host, port))
             return True
         except socket.error:
             return False
         finally:
             s.close()
 
-    def wait_for_port(self, timeout=None):
-        timeout = timeout or self.DEFAULT_STARTUP_TIMEOUT
-        return transport.wait_for_port(self.host, self.port, timeout=timeout)
+    def _wait_for_connection(self, timeout=None):
+        """Wait until Marionette server has been created the communication socket.
+
+        :param timeout: Timeout in seconds for the server to be ready.
+
+        """
+        if timeout is None:
+            timeout = self.DEFAULT_STARTUP_TIMEOUT
+
+        runner = None
+        if self.instance is not None:
+            runner = self.instance.runner
+
+        poll_interval = 0.1
+        starttime = datetime.datetime.now()
+
+        while datetime.datetime.now() - starttime < datetime.timedelta(seconds=timeout):
+            # If the instance we want to connect to is not running return immediately
+            if runner is not None and not runner.is_running():
+                return False
+
+            sock = None
+            try:
+                sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
+                sock.settimeout(0.5)
+                sock.connect((self.host, self.port))
+                data = sock.recv(16)
+                if ":" in data:
+                    return True
+            except socket.error:
+                pass
+            finally:
+                if sock is not None:
+                    sock.close()
+
+            time.sleep(poll_interval)
+
+        return False
 
     @do_process_check
-    def raise_for_port(self, port_obtained):
-        if not port_obtained:
-            raise socket.timeout("Timed out waiting for port {}!".format(self.port))
+    def raise_for_connection(self, timeout=None):
+        """Raise socket.timeout if no connection can be established.
+
+        :param timeout: Timeout in seconds for the server to be ready.
+
+        """
+        if not self._wait_for_connection(timeout):
+            raise socket.timeout("Timed out waiting for connection on {0}:{1}!".format(
+                self.host, self.port))
 
     @do_process_check
     def _send_message(self, name, params=None, key=None):
         """Send a blocking message to the server.
 
         Marionette provides an asynchronous, non-blocking interface and
         this attempts to paper over this by providing a synchronous API
         to the user.
@@ -1027,17 +1069,17 @@ class Marionette(object):
             }}
             """.format(pref, value))
             if not pref_exists:
                 break
         self.set_context(self.CONTEXT_CONTENT)
         if not pref_exists:
             self.delete_session()
             self.instance.restart(prefs)
-            self.raise_for_port(self.wait_for_port())
+            self.raise_for_connection()
             self.start_session()
             self.reset_timeouts()
 
     def _request_in_app_shutdown(self, shutdown_flags=None):
         """Terminate the currently running instance from inside the application.
 
         :param shutdown_flags: If specified use additional flags for the shutdown
                                of the application. Possible values here correspond
@@ -1120,27 +1162,27 @@ class Marionette(object):
                 raise ValueError("An in_app restart cannot be triggered with the clean flag set")
 
             if callable(callback):
                 callback()
             else:
                 self._request_in_app_shutdown("eRestart")
 
             try:
-                self.raise_for_port(self.wait_for_port())
+                self.raise_for_connection()
             except socket.timeout:
                 if self.instance.runner.returncode is not None:
                     exc, val, tb = sys.exc_info()
                     self.cleanup()
                     raise exc, "Requested restart of the application was aborted", tb
 
         else:
             self.delete_session()
             self.instance.restart(clean=clean)
-            self.raise_for_port(self.wait_for_port())
+            self.raise_for_connection()
 
         self.start_session(session_id=session_id)
         self.reset_timeouts()
 
         if in_app and self.session.get("processId"):
             # In some cases Firefox restarts itself by spawning into a new process group.
             # As long as mozprocess cannot track that behavior (bug 1284864) we assist by
             # informing about the new process id.
@@ -1173,19 +1215,19 @@ class Marionette(object):
                 # We're managing a binary which has terminated, so restart it.
                 self.instance.restart()
 
         self.client = transport.TcpTransport(
             self.host,
             self.port,
             self.socket_timeout)
 
-        # Call wait_for_port() before attempting to connect in
+        # Call _wait_for_connection() before attempting to connect in
         # the event gecko hasn't started yet.
-        self.wait_for_port(timeout=timeout)
+        self._wait_for_connection(timeout=timeout)
         self.protocol, _ = self.client.connect()
 
         body = {"capabilities": desired_capabilities, "sessionId": session_id}
         resp = self._send_message("newSession", body)
 
         self.session_id = resp["sessionId"]
         self.session = resp["value"] if self.protocol == 1 else resp["capabilities"]
 
--- a/testing/marionette/client/marionette_driver/transport.py
+++ b/testing/marionette/client/marionette_driver/transport.py
@@ -1,13 +1,12 @@
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
-import datetime
 import json
 import socket
 import time
 
 
 class SocketTimeout(object):
     def __init__(self, socket, timeout):
         self.sock = socket
@@ -278,32 +277,8 @@ class TcpTransport(object):
         """Close the socket."""
         if self.sock:
             self.sock.shutdown(socket.SHUT_RDWR)
             self.sock.close()
             self.sock = None
 
     def __del__(self):
         self.close()
-
-
-def wait_for_port(host, port, timeout=60):
-    """Wait for the specified host/port to become available."""
-    starttime = datetime.datetime.now()
-    poll_interval = 0.1
-    while datetime.datetime.now() - starttime < datetime.timedelta(seconds=timeout):
-        sock = None
-        try:
-            sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
-            sock.settimeout(0.5)
-            sock.connect((host, port))
-            data = sock.recv(16)
-            sock.shutdown(socket.SHUT_RDWR)
-            sock.close()
-            if ":" in data:
-                return True
-        except socket.error:
-            pass
-        finally:
-            if sock is not None:
-                sock.close()
-        time.sleep(poll_interval)
-    return False
--- a/testing/marionette/harness/marionette/tests/unit/test_marionette.py
+++ b/testing/marionette/harness/marionette/tests/unit/test_marionette.py
@@ -1,30 +1,40 @@
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 import itertools
+import time
+
+from marionette.marionette_test import MarionetteTestCase
 
 from marionette_driver import errors
-from marionette.marionette_test import MarionetteTestCase
 
 
-class TestMarionetteProperties(MarionetteTestCase):
+class TestMarionette(MarionetteTestCase):
 
     def test_correct_test_name(self):
         """Test that the correct test name gets set."""
         expected_test_name = '{module}.py {cls}.{func}'.format(
             module=__name__,
             cls=self.__class__.__name__,
             func=self.test_correct_test_name.__name__,
         )
 
         self.assertEqual(self.marionette.test_name, expected_test_name)
 
+    def test_wait_for_connection_non_existing_process(self):
+        """Test that wait_for_port doesn't run into a timeout if instance is not running."""
+        self.marionette.quit()
+        self.assertIsNotNone(self.marionette.instance.runner.returncode)
+        start_time = time.time()
+        self.assertFalse(self.marionette._wait_for_connection(timeout=5))
+        self.assertLess(time.time() - start_time, 5)
+
 
 class TestProtocol1Errors(MarionetteTestCase):
     def setUp(self):
         MarionetteTestCase.setUp(self)
         self.op = self.marionette.protocol
         self.marionette.protocol = 1
 
     def tearDown(self):