Make TimeoutRetryThread's stoppable

build/android/pylib/device/device_utils.py

 # Copyright 2014 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """Provides a variety of device interactions based on adb.
 
 Eventually, this will be based on adb_wrapper.
 """
 # pylint: disable=W0613
 
 import logging
 import multiprocessing
 import os
 import re
 import sys
 import tempfile
+import threading
 import time
 import zipfile
 
 import pylib.android_commands
 from pylib import cmd_helper
 from pylib.device import adb_wrapper
 from pylib.device import decorators
 from pylib.device import device_errors
 from pylib.device.commands import install_commands
 from pylib.utils import apk_helper
@@ skipping 186 matching lines @@
     Args:
       wifi: A boolean indicating if we should wait for wifi to come up or not.
       timeout: timeout in seconds
       retries: number of retries
 
     Raises:
       CommandFailedError on failure.
       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
 
   @decorators.WithTimeoutAndRetriesDefaults(
       REBOOT_DEFAULT_TIMEOUT,
       REBOOT_DEFAULT_RETRIES)
   def Reboot(self, block=True, timeout=None, retries=None):
     """Reboot the device.
 
@@ skipping 142 matching lines @@
     except device_errors.AdbShellCommandFailedError as e:
       if check_return:
         raise
       else:
         output = e.output
 
     output = output.splitlines()
     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
 
   @decorators.WithTimeoutAndRetriesFromInstance()
   def KillAll(self, process_name, signum=9, as_root=False, blocking=False,
               timeout=None, retries=None):
     """Kill all processes with the given name on the device.
 
     Args:
@@ skipping 479 matching lines @@
 
     Returns:
       True if the device-side property changed and a restart is required as a
       result, False otherwise.
 
     Raises:
       CommandTimeoutError on timeout.
     """
     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
 
     Returns:
       The value of the device's |property_name| property.
 
     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:
       property_name: A string containing the name of the property to set on
                      the device.
       value: A string containing the value to set to the property on the
              device.
       timeout: timeout in seconds
       retries: number of retries
 
     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):
     """Gets the device main ABI.
 
     Args:
       timeout: timeout in seconds
       retries: number of retries
 
     Returns:
@@ skipping 118 matching lines @@
     Returns:
       A Parallelizer operating over |devices|.
     """
     if not devices or len(devices) == 0:
       devices = pylib.android_commands.GetAttachedDevices()
     parallelizer_type = (parallelizer.Parallelizer if async
                          else parallelizer.SyncParallelizer)
     return parallelizer_type([
         d if isinstance(d, DeviceUtils) else DeviceUtils(d)
         for d in devices])