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 74 matching lines @@
     Args:
       timeout: timeout in seconds
       retries: number of retries
 
     Returns:
       True if the device is online, False otherwise.
 
     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):
     """Checks whether or not adbd has root privileges.
 
     Args:
       timeout: timeout in seconds
       retries: number of retries
 
     Returns:
@@ skipping 37 matching lines @@
       retries: number of retries
 
     Returns:
       True if the device is running a user build, False otherwise (i.e. if
         it's running a userdebug build).
 
     Raises:
       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):
     """Get the device's path to its SD card.
 
     Args:
       timeout: timeout in seconds
       retries: number of retries
 
     Returns:
@@ skipping 30 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()],
+                                  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
 
   @decorators.WithTimeoutAndRetriesDefaults(
       REBOOT_DEFAULT_TIMEOUT,
       REBOOT_DEFAULT_RETRIES)
   def Reboot(self, block=True, timeout=None, retries=None):

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

This brings in some of the logic from android_commands.Reboot. I did not
include, however, the check of whether we're running on HIVE [1] (apparently a
now defunct project?)

[1]:
https://cs.corp.google.com/#clankium/src/build/android/pylib/android_commands...

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

public cs:
https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli...

I'm guessing you asked torne about this?

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

Just checked with him. HIVE is not used anymore, so it's OK to remove anything
that deals with it.

     """Reboot the device.
 
     Args:
       block: A boolean indicating if we should wait for the reboot to complete.
       timeout: timeout in seconds
       retries: number of retries
 
     Raises:
       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
 
   @decorators.WithTimeoutAndRetriesDefaults(
       INSTALL_DEFAULT_TIMEOUT,
       INSTALL_DEFAULT_RETRIES)
   def Install(self, apk_path, reinstall=False, timeout=None, retries=None):
     """Install an APK.
 
@@ skipping 120 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 300 matching lines @@
       try:
         self.adb.Push(zip_file.name, zip_on_device)
         self._RunShellCommandImpl(
             ['unzip', zip_on_device],
             as_root=True,
             env={'PATH': '$PATH:%s' % install_commands.BIN_DIR},
             check_return=True)
       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
   def _CreateDeviceZip(zip_path, host_device_tuples):
     with zipfile.ZipFile(zip_path, 'w') as zip_file:
       for host_path, device_path in host_device_tuples:
         if os.path.isfile(host_path):
           zip_file.write(host_path, device_path, zipfile.ZIP_DEFLATED)
         else:
           for hd, _, files in os.walk(host_path):
@@ skipping 158 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):
+  @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
 
     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:
+      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:
       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], 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):
     """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])