Bug 1290372 - wait_for_port() has to return early if instance is not running. draft
authorHenrik Skupin <mail@hskupin.info>
Mon, 17 Oct 2016 15:38:07 +0200
changeset 425995 aa039aa626b41861b7794fd4d03ad24fe034c0a4
parent 425227 1391a2889aeb2bdd61ad6ef838e65826e35aabc2
child 534044 b61cb5b801e7bd5bae2d93c68f981022111ecc25
push id32566
push userbmo:hskupin@gmail.com
push dateMon, 17 Oct 2016 13:40:32 +0000
bugs1290372
milestone52.0a1
Bug 1290372 - wait_for_port() has to return early if instance is not running. 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
 
@@ -570,17 +572,17 @@ class Marionette(object):
         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_port(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:
@@ -635,23 +637,64 @@ class Marionette(object):
             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)
+        """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_port(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_port(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 +1070,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_port()
             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 +1163,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_port()
             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_port()
 
         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.
--- 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_port_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_port(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):