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..14ee6623da49e1d770051a8ea23c6420c0b03548 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
 
@@ -220,18 +221,50 @@ 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()])
 
-  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):
+    start = time.time()
+    while not threading.current_thread().Stopped():
+      try:
+        r = condition()
+      except device_errors.CommandFailedError as e:
+        r = False

[jbudorick, 2014/10/24 17:30:51]

nit: similarly here w.r.t False v None

+        logging.warning('%s at %s', str(e), threading.current_thread().name)
+      elapsed = time.time() - start
+      if r:
+        logging.warning('%s: condition %r met (%1.2f) at %s', str(self),

[jbudorick, 2014/10/24 17:30:51]

nit: should be info or debug, not warning

[perezju, 2014/10/27 11:07:07]

Yes, I agree these should be info. At the moment this was just a temporary thing
for me to debug while developing. The problem was tht setting the logger level
to info already dumps too much to the screen because of cmd_helper logging every
single shell command run on the host.

+                     condition.__name__, elapsed, threading.current_thread().name)
+        return r
+      time.sleep(wait_period)
+      logging.warning('%s: condition %r not yet met (%1.2f) at %s', str(self),
+                   condition.__name__, elapsed, threading.current_thread().name)
+    return False

[jbudorick, 2014/10/24 17:30:51]

nit: I think this should return None, as False implies (to me at least) that
we're otherwise returning true. We're returning the result of the condition,
which isn't necessarily True/False

[perezju, 2014/10/27 11:07:06]

Agree.

+
+  @decorators.WithTimeoutAndRetriesFromInstance()
+  def GetApplicationPath(self, package, timeout=None, retries=None):
+    output = self.RunShellCommand(['pm', 'path', package], single_line=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
@@ -394,7 +427,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
@@ -894,13 +927,13 @@ class DeviceUtils(object):
     """
     return self.old_interface.SetJavaAssertsEnabled(enabled)
 
-  @decorators.WithTimeoutAndRetriesFromInstance()
-  def GetProp(self, property_name, timeout=None, retries=None):
+  def GetProp(self, property_name, cache=False, timeout=None, retries=None):

[jbudorick, 2014/10/24 17:30:51]

We should make sure we test the timeout/retries part of this pretty well.

[perezju, 2014/10/27 11:07:07]

Acknowledged.

     """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 +943,21 @@ 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:

[jbudorick, 2014/10/24 17:30:51]

I think that, for now, we should limit property caching to what we were doing
before (ro.boot.*, ro.build.*, and ro.product.*). We can look at expanding that
list separately. See b/a/pylib/system_properties.py.

[perezju, 2014/10/27 11:07:07]

An idea I had was to provide the class with some properties such as: build_id,
build_type, product_name, etc; which are implemented as calls to GetProp with
cache=True. Thoughts?

I'm undecided about this. Maybe the best behavior is indeed for GetProp to
already "know" which properties should be cached as currently done?

+      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,
+                                   timeout=timeout, retries=retries)
+      if cache or cache_name in self._cache:
+        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,

[jbudorick, 2014/10/24 17:30:51]

What's the motivation behind check?

[perezju, 2014/10/27 11:07:07]

The TODO here
https://cs.corp.google.com/#clankium/src/build/android/pylib/system_propertie...

Without root on the device, trying to setprop just fails silently (no error
messages on the output nor non-zero exit code).

I actually think this shouldn't be an option, and the check should always be
made. But also didn't want to add a second shell call all the time. (Or maybe I
should make a small shell script that sets and checks?)

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

Use the public cs for upstream reviews:
https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli...
That makes sense to me, but we should leave it as a default off option in this
CL. Add a TODO to remove the option, make the check mandatory, and rework it as
a shell script that does setprop & getprop.

+              retries=None):
     """Sets a property on the device.
 
     Args:
@@ -930,7 +971,11 @@ class DeviceUtils(object):
     Raises:
       CommandTimeoutError on timeout.
     """
-    self.old_interface.system_properties[property_name] = value
+    self.RunShellCommand(['setprop', property_name, value])

[jbudorick, 2014/10/24 17:30:51]

If we expand caching beyond read-only properties, we should update the cache
here.

[perezju, 2014/10/27 11:07:06]

This happens "auto-magically" when check=True. Maybe we should just always do
the check? (The alternative is that in some cases we might end up storing
something in the cache which didn't actually made it into the device)

+    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))
 
   @decorators.WithTimeoutAndRetriesFromInstance()
   def GetABI(self, timeout=None, retries=None):