Make TimeoutRetryThread's stoppable

build/android/pylib/device/device_utils.py

Index: build/android/pylib/device/device_utils.py
diff --git a/build/android/pylib/device/device_utils.py b/build/android/pylib/device/device_utils.py
index b2fa1ff3953a702f6cd3676f495db89144dcf0d7..4cd255c3cea3e5a24248a5d3a7028335b432ef99 100644
--- a/build/android/pylib/device/device_utils.py
+++ b/build/android/pylib/device/device_utils.py
@@ -14,6 +14,7 @@ import os
 import re
 import sys
 import tempfile
+import threading
 import time
 import zipfile
 
@@ -108,10 +109,8 @@ class DeviceUtils(object):
     Raises:
       CommandTimeoutError on timeout.
     """
-    return self._IsOnlineImpl()
-
-  def _IsOnlineImpl(self):
-    return self.old_interface.IsOnline()
+    state = self.adb.GetState()
+    return state == 'device'
 
   @decorators.WithTimeoutAndRetriesFromInstance()
   def HasRoot(self, timeout=None, retries=None):
@@ -169,7 +168,7 @@ class DeviceUtils(object):
       CommandTimeoutError on timeout.
       DeviceUnreachableError on missing device.
     """
-    return self._GetPropImpl('ro.build.type') == 'user'
+    return self.build_type == 'user'

[perezju, 2014/10/27 17:43:34]

This would be a property of the class, see below.

 
   @decorators.WithTimeoutAndRetriesFromInstance()
   def GetExternalStoragePath(self, timeout=None, retries=None):
@@ -220,18 +219,56 @@ class DeviceUtils(object):
       CommandTimeoutError if one of the component waits times out.
       DeviceUnreachableError if the device becomes unresponsive.
     """
-    self._WaitUntilFullyBootedImpl(wifi=wifi, timeout=timeout)
+    def sd_card_ready():
+      return self.RunShellCommand(['ls', self.GetExternalStoragePath()],
+                                  single_line=True, check_return=True)
 
-  def _WaitUntilFullyBootedImpl(self, wifi=False, timeout=None):
-    if timeout is None:
-      timeout = self._default_timeout
-    self.old_interface.WaitForSystemBootCompleted(timeout)
-    self.old_interface.WaitForDevicePm()
-    self.old_interface.WaitForSdCardReady(timeout)
+    def pm_ready():
+      return self.GetApplicationPath('android')
+
+    def boot_completed():
+      return self.GetProp('sys.boot_completed') == '1'
+
+    def wifi_enabled():
+      return 'Wi-Fi is enabled' in self.RunShellCommand(['dumpsys', 'wifi'])
+
+    self.adb.WaitForDevice()
+    self._WaitFor(sd_card_ready)
+    self._WaitFor(pm_ready)
+    self._WaitFor(boot_completed)
     if wifi:
-      while not 'Wi-Fi is enabled' in (
-          self.old_interface.RunShellCommand('dumpsys wifi')):
-        time.sleep(1)
+      self._WaitFor(wifi_enabled)
+
+  def _WaitFor(self, condition, wait_period=5, max_tries=None):
+    thread = threading.current_thread()
+    while max_tries is None or max_tries > 0:
+      try:
+        r = condition()
+      except device_errors.CommandFailedError:
+        r = None
+      thread.CheckTimeout()

[perezju, 2014/10/27 17:43:34]

This would raise an exception if the current thread should have timed out by
now. Should be added after any potentially lengthy operation.

[jbudorick, 2014/10/27 19:15:36]

I really don't like delegating this responsibility to the client implementation
-- see my comment in reraiser_thread.

+      if max_tries is not None:
+        max_tries -= 1
+      if r:
+        logging.info('(device: %s) condition %r met (%1.2fs) at %s',
+            str(self), condition.__name__, thread.ElapsedTime(), thread.name)
+        return r
+      else:
+        logging.info('(device: %s) condition %r not yet met (%1.2fs) at %s',
+            str(self), condition.__name__, thread.ElapsedTime(), thread.name)
+        time.sleep(wait_period)
+        thread.CheckTimeout()
+    return None
+
+  @decorators.WithTimeoutAndRetriesFromInstance()
+  def GetApplicationPath(self, package, timeout=None, retries=None):
+    output = self.RunShellCommand(['pm', 'path', package], single_line=True,
+                                  check_return=True)
+    if not output.startswith('package:'):
+      raise device_errors.CommandFailedError('pm path returned: %r' % output,
+                                             str(self))
+    return output[len('package:'):]
+
 
   REBOOT_DEFAULT_TIMEOUT = 10 * _DEFAULT_TIMEOUT
   REBOOT_DEFAULT_RETRIES = _DEFAULT_RETRIES
@@ -251,9 +288,14 @@ class DeviceUtils(object):
       CommandTimeoutError on timeout.
       DeviceUnreachableError on missing device.
     """
-    self.old_interface.Reboot()
+    def device_offline():

[jbudorick, 2014/10/27 19:15:36]

nit: for something this simple, just use a lambda inline.

+      return not self.IsOnline()
+
+    self.adb.Reboot()
+    self._cache = {}
+    self._WaitFor(device_offline, wait_period=1, max_tries=5)

[jbudorick, 2014/10/27 19:15:36]

With the lambda, this becomes:

  self._WaitFor(lambda: not self.IsOnline(), ...)

[perezju, 2014/10/28 14:31:17]

Yes, but if we use a named function then _WaitFor can also use the more
descriptive label "device_offline" in log messages.

[jbudorick, 2014/10/28 17:28:46]

Ah. Stick with device_offline, then.

     if block:
-      self._WaitUntilFullyBootedImpl(timeout=timeout)
+      self.WaitUntilFullyBooted()
 
   INSTALL_DEFAULT_TIMEOUT = 4 * _DEFAULT_TIMEOUT
   INSTALL_DEFAULT_RETRIES = _DEFAULT_RETRIES
@@ -394,7 +436,7 @@ class DeviceUtils(object):
     if single_line:
       if len(output) != 1:
         msg = 'exactly one line of output expected, but got: %s'
-        raise device_errors.CommandFailedError(msg % output)
+        raise device_errors.CommandFailedError(msg % output, str(self))
       return output[0]
     else:
       return output
@@ -715,7 +757,7 @@ class DeviceUtils(object):
       finally:
         if zip_proc.is_alive():
           zip_proc.terminate()
-        if self._IsOnlineImpl():
+        if self.IsOnline():
           self._RunShellCommandImpl(['rm', zip_on_device], check_return=True)
 
   @staticmethod
@@ -894,13 +936,17 @@ class DeviceUtils(object):
     """
     return self.old_interface.SetJavaAssertsEnabled(enabled)
 
-  @decorators.WithTimeoutAndRetriesFromInstance()
-  def GetProp(self, property_name, timeout=None, retries=None):
+  @property
+  def build_type(self):
+    return self.GetProp('ro.build.type', cache=True)

[perezju, 2014/10/27 17:43:34]

This is the sort of thing that I propose to both: cache properties which make
sense to cache, and provide clients with simple access to them.

[jbudorick, 2014/10/27 19:15:36]

I'll have to think about this. It bloats the interface some, but clients don't
have to worry about the name of the particular android property they're
interested in. Leaning yes at the moment, though.

In general, what do you think would be the threshold for switching from GetProp
to a property?

[perezju, 2014/10/28 14:31:18]

I say we could start with properties that (1) are read-only (so makes sense to
cache them) and (2) clients actually want to access them.

Currently this means (grepping through uses of GetProp and system_properties):

ro.build.description
ro.build.fingerprint
ro.build.id
ro.build.product
ro.build.type
ro.build.version.sdk
ro.product.cpu.abi
ro.product.model
ro.product.name
ro.setupwizard.mode

[jbudorick, 2014/10/28 17:28:45]

Sounds good to me.

+
+  def GetProp(self, property_name, cache=False, timeout=None, retries=None):
     """Gets a property from the device.
 
     Args:
       property_name: A string containing the name of the property to get from
-                     the device.
+        the device.
+      cache: A boolean indicating whether to cache the value of this property.
       timeout: timeout in seconds
       retries: number of retries
 
@@ -910,13 +956,22 @@ class DeviceUtils(object):
     Raises:
       CommandTimeoutError on timeout.
     """
-    return self._GetPropImpl(property_name)
-
-  def _GetPropImpl(self, property_name):
-    return self.old_interface.system_properties[property_name]
+    cache_name = '_prop:' + property_name
+    if cache and cache_name in self._cache:
+      return self._cache[cache_name]
+    else:
+      # timeout and retries are handled down at run shell, because we don't
+      # want to apply them in the other branch when reading from the cache
+      value = self.RunShellCommand(['getprop', property_name],
+                                   single_line=True, check_return=True,
+                                   timeout=timeout, retries=retries)
+      if cache or cache_name in self._cache:

[jbudorick, 2014/10/27 19:15:36]

The second clause here is interesting. Should we update the cache or remove the
item from the cache?

[perezju, 2014/10/28 14:31:18]

I say we update. From the point of view of the caller, cache=False means "I want
to get a real value directly from the device", while cache=True means "I'm happy
to get a cached value from a previous call, if any". Updating here seems less
wasteful and keeps both types of callers happy.

[jbudorick, 2014/10/28 17:28:45]

Makes sense.

+        self._cache[cache_name] = value
+      return value
 
   @decorators.WithTimeoutAndRetriesFromInstance()
-  def SetProp(self, property_name, value, timeout=None, retries=None):
+  def SetProp(self, property_name, value, check=False, timeout=None,
+              retries=None):
     """Sets a property on the device.
 
     Args:
@@ -930,7 +985,11 @@ class DeviceUtils(object):
     Raises:
       CommandTimeoutError on timeout.
     """
-    self.old_interface.system_properties[property_name] = value
+    self.RunShellCommand(['setprop', property_name, value], check_return=True)
+    if check and value != self.GetProp(property_name):
+      raise device_errors.CommandFailedError(
+          'Unable to set property %r on the device to %r'
+          % (property_name, value), str(self))

[perezju, 2014/10/27 17:43:34]

Still wondering whether to always make the check, and pack these two shell calls
in a single script?

[jbudorick, 2014/10/27 19:15:36]

Not yet. In a separate CL, definitely.

 
   @decorators.WithTimeoutAndRetriesFromInstance()
   def GetABI(self, timeout=None, retries=None):