Repository: google/mobly

Issue:
Rpc socket times out before EventHandler.waitAndGet
`json_rpc_base._SOCKET_TIMEOUT` is 60s. Yet `EventHandler.waitAndGet` can take an arbitrary timeout length.

`EventHandler.waitAndGet` makes a call to `EventSnippet.eventWaitAndGet` through the socket, which doesn't return until the event is received or the method itself times out.

So if somebody calls `EventHandler.waitAndGet` with a timeout longer than `json_rpc_base._SOCKET_TIMEOUT`, we'd get a socket timeout error.

We should:
1. Make socket timeout longer
2. Add a guard in `EventHandler.waitAndGet` for a max timeout value.

Discussion:

Patch:
diff --git a/mobly/controllers/android_device_lib/callback_handler.py b/mobly/controllers/android_device_lib/callback_handler.py
index 7c648c2..ddcf029 100644
--- a/mobly/controllers/android_device_lib/callback_handler.py
+++ b/mobly/controllers/android_device_lib/callback_handler.py
@@ -18,6 +18,11 @@ import time
 
 from mobly.controllers.android_device_lib import snippet_event
 
+# The max timeout cannot be larger than the max time the socket waits for a
+# response message. Otherwise, the socket would timeout before the Rpc call
+# does, leaving both server and client in unknown states.
+MAX_TIMEOUT = 60 * 10
+
 
 class Error(Exception):
     pass
@@ -68,9 +73,15 @@ class CallbackHandler(object):
             SnippetEvent, the oldest entry of the specified event.
 
         Raises:
+            Error: If the specified timeout is longer than the max timeout
+                   supported.
             TimeoutError: The expected event does not occur within time limit.
         """
         if timeout:
+            if timeout > MAX_TIMEOUT:
+                raise Error(
+                    'Specified timeout %s is longer than max timeout %s.' %
+                    (timeout, MAX_TIMEOUT))
             timeout *= 1000  # convert to milliseconds for java side
         try:
             raw_event = self._event_client.eventWaitAndGet(self._id,
@@ -78,8 +89,8 @@ class CallbackHandler(object):
         except Exception as e:
             if 'EventSnippetException: timeout.' in str(e):
                 raise TimeoutError(
-                    'Timeout waiting for event "%s" triggered by %s (%s).'
-                    % (event_name, self._method_name, self._id))
+                    'Timeout waiting for event "%s" triggered by %s (%s).' %
+                    (event_name, self._method_name, self._id))
             raise
         return snippet_event.from_dict(raw_event)
 
diff --git a/mobly/controllers/android_device_lib/jsonrpc_client_base.py b/mobly/controllers/android_device_lib/jsonrpc_client_base.py
index bb4b5ed..5b661a3 100644
--- a/mobly/controllers/android_device_lib/jsonrpc_client_base.py
+++ b/mobly/controllers/android_device_lib/jsonrpc_client_base.py
@@ -53,7 +53,10 @@ APP_START_WAIT_TIME = 15
 UNKNOWN_UID = -1
 
 # Maximum time to wait for the socket to open on the device.
-_SOCKET_TIMEOUT = 60
+_SOCKET_CONNECTION_TIMEOUT = 60
+
+# Maximum time to wait for a response message on the socket.
+_SOCKET_READ_TIMEOUT = callback_handler.MAX_TIMEOUT
 
 
 class Error(Exception):
@@ -70,9 +73,9 @@ class ApiError(Error):
 
 class ProtocolError(Error):
     """Raised when there is some error in exchanging data with server."""
-    NO_RESPONSE_FROM_HANDSHAKE = "No response from handshake."
-    NO_RESPONSE_FROM_SERVER = "No response from server."
-    MISMATCHED_API_ID = "Mismatched API id."
+    NO_RESPONSE_FROM_HANDSHAKE = 'No response from handshake.'
+    NO_RESPONSE_FROM_SERVER = 'No response from server.'
+    MISMATCHED_API_ID = 'Mismatched API id.'
 
 
 class JsonRpcCommand(object):
@@ -186,9 +189,9 @@ class JsonRpcClientBase(object):
         """Opens a connection to a JSON RPC server.
 
         Opens a connection to a remote client. The connection attempt will time
-        out if it takes longer than _SOCKET_TIMEOUT seconds. Each subsequent
-        operation over this socket will time out after _SOCKET_TIMEOUT seconds
-        as well.
+        out if it takes longer than _SOCKET_CONNECTION_TIMEOUT seconds. Each
+        subsequent operation over this socket will time out after
+        _SOCKET_READ_TIMEOUT seconds as well.
 
         Args:
             uid: int, The uid of the session to join, or UNKNOWN_UID to start a
@@ -202,8 +205,8 @@ class JsonRpcClientBase(object):
         """
         self._counter = self._id_counter()
         self._conn = socket.create_connection(('127.0.0.1', self.host_port),
-                                              _SOCKET_TIMEOUT)
-        self._conn.settimeout(_SOCKET_TIMEOUT)
+                                              _SOCKET_CONNECTION_TIMEOUT)
+        self._conn.settimeout(_SOCKET_READ_TIMEOUT)
         self._client = self._conn.makefile(mode='brw')
 
         resp = self._cmd(cmd, uid)
diff --git a/mobly/controllers/android_device_lib/sl4a_client.py b/mobly/controllers/android_device_lib/sl4a_client.py
index ef5583e..ba31e0f 100644
--- a/mobly/controllers/android_device_lib/sl4a_client.py
+++ b/mobly/controllers/android_device_lib/sl4a_client.py
@@ -57,4 +57,4 @@ class Sl4aClient(jsonrpc_client_base.JsonRpcClientBase):
             "pm list package | grep com.googlecode.android_scripting"):
             raise jsonrpc_client_base.AppStartError(
                 '%s is not installed on %s' % (
-                self.app_name, self._adb.getprop('ro.boot.serialno')))
+                self.app_name, self._adb.serial))
diff --git a/mobly/controllers/android_device_lib/snippet_client.py b/mobly/controllers/android_device_lib/snippet_client.py
index 7c3ca01..793e3ae 100644
--- a/mobly/controllers/android_device_lib/snippet_client.py
+++ b/mobly/controllers/android_device_lib/snippet_client.py
@@ -56,8 +56,8 @@ class SnippetClient(jsonrpc_client_base.JsonRpcClientBase):
             app_name=package,
             adb_proxy=adb_proxy)
         self.package = package
-        self._serial = self._adb.getprop('ro.boot.serialno')
         self.log = log
+        self._serial = self._adb.serial
 
     def _do_start_app(self):
         """Overrides superclass."""


Test patch:
diff --git a/tests/mobly/controllers/android_device_lib/callback_handler_test.py b/tests/mobly/controllers/android_device_lib/callback_handler_test.py
index d121fe8..1a3afc0 100755
--- a/tests/mobly/controllers/android_device_lib/callback_handler_test.py
+++ b/tests/mobly/controllers/android_device_lib/callback_handler_test.py
@@ -37,6 +37,10 @@ class CallbackHandlerTest(unittest.TestCase):
     """Unit tests for mobly.controllers.android_device_lib.callback_handler.
     """
 
+    def test_timeout_value(self):
+        self.assertGreaterEqual(jsonrpc_client_base._SOCKET_READ_TIMEOUT,
+                                callback_handler.MAX_TIMEOUT)
+
     def test_event_dict_to_snippet_event(self):
         mock_event_client = mock.Mock()
         mock_event_client.eventWaitAndGet = mock.Mock(