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'

[jbudorick, 2014/10/30 02:27:03]

return self.adb.getState() == 'device'

?

[perezju, 2014/10/30 18:44:45]

:S .. probably a left-over from some (now missing) debug code. Fixed now.

 
   @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'
 
   @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 13 matching lines @@
     value = self._RunShellCommandImpl('echo $EXTERNAL_STORAGE',
                                       single_line=True,
                                       check_return=True)
     if not value:
       raise device_errors.CommandFailedError('$EXTERNAL_STORAGE is not set',
                                              str(self))
     self._cache['external_storage'] = value
     return value
 
   @decorators.WithTimeoutAndRetriesFromInstance()
+  def GetApplicationPath(self, package, timeout=None, retries=None):
+    """Get the path of the installed apk on the device for the given package.
+
+    Args:
+      package: Name of the package.
+
+    Returns:
+      Path to the apk on the device if it exists, None otherwise.
+    """
+    output = self.RunShellCommand(['pm', 'path', package], single_line=True,
+                                  check_return=True)
+    if not output:
+      return None
+    if not output.startswith('package:'):
+      raise device_errors.CommandFailedError('pm path returned: %r' % output,
+                                             str(self))
+    return output[len('package:'):]
+
+  @decorators.WithTimeoutAndRetriesFromInstance()
   def WaitUntilFullyBooted(self, wifi=False, timeout=None, retries=None):
     """Wait for the device to fully boot.
 
     This means waiting for the device to boot, the package manager to be
     available, and the SD card to be ready. It can optionally mean waiting
     for wifi to come up, too.
 
     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)

[perezju, 2014/10/29 18:16:50]

Note that this might throw a CommandFailedError if GetExternalStoragePath fails
(the path is not set). Maybe this is what we want, because it doesn't make much
sense to "wait" for this to change? (Hmm, actually not sure about what happens
when the sd card is not ready, I've never seen this happen)

[jbudorick, 2014/10/30 02:27:03]

I've never seen this either, so I have no idea.

 
-  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():
+      try:
+        return self.GetApplicationPath('android')
+      except device_errors.CommandFailedError:
+        return False

[perezju, 2014/10/29 18:16:50]

We need to catch CommandFailedError here because this is what we get when the pm
is not ready (something like "Error. Is the package manager running?)

+
+    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):
+    """Wait for a condition to become true.
+
+    Repeadly call the function condition(), with no arguments, until it returns
+    a true value.
+
+    This method assumes that it is being called within a TimeoutRetryThread.
+
+    Args:
+      condition: function with the condition to check
+      wait_period: how long to wait before retrying to check the condition
+      max_tries: maximum number of checks to make, the default tries forever
+        (usually until a timeout error is raised)
+
+    Returns:
+      The true value returned by the condition, or None if the condition was
+      not met after max_tries.
+    """
+    thread = threading.current_thread()
+    while max_tries is None or max_tries > 0:
+      r = condition()
+      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)

[jbudorick, 2014/10/30 02:27:03]

no need for the thread name, we already print it when we log. If you look at the
provision_devices stdio from your trybot run, you'll see these:

I   44.300s TimeoutThread-1-for-ProvisionDevice(009eabcbcada0bae)  (device:
009eabcbcada0bae) condition 'pm_ready' not yet met (42.99s) at
TimeoutThread-1-for-ProvisionDevice(009eabcbcada0bae)

[perezju, 2014/10/30 18:44:45]

Looking at that message, we don't need to include the device serial either,
which was essentially the only use of "self" here. This is what lead me to think
about a more general function better placed at timeout_retry.WaitFor. Have a
look at it on my new proposal.

+        return r
+      else:
+        logging.info('(device: %s) condition %r not yet met (%1.2fs) at %s',
+            str(self), condition.__name__, thread.ElapsedTime(), thread.name)

[jbudorick, 2014/10/30 02:27:03]

ditto

+        self.adb.Wait(wait_period)
+    return None
 
   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.
 
     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():
+      return not self.IsOnline()
+
+    self.adb.Reboot()
+    self._cache = {}
+    self._WaitFor(device_offline, wait_period=1, max_tries=5)
     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 118 matching lines @@
       # adb.Shell without a timeout-and-retries wrapper.
       output = self.adb.Shell(cmd, expect_rc=0, timeout=timeout, retries=0)
     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)
-      return output[0]
+      if not output:
+        return ''

[jbudorick, 2014/10/30 02:27:03]

What's up with this? If a caller requests a single line but gets no lines, why
shouldn't it be an error...?

[perezju, 2014/10/30 18:44:45]

I explained this on a previous review of the code, but I had another thought
about it again anyway.

The issue is that some commands, like "pm path <package>", return with no output
if the package is not installed, and clients of GetApplicationPath expect a
return value of None in this case (which makes a lot of sense).

If we raise an error, then callers have a hard time figuring out whether the
error was raised due to empty output, or because the output consisted, e.g., of
a large dump of data due to some other error (which should genuinely raise all
sorts of error alarms!).

So I think that returning an empty string when the output is empty is the most
useful approach. Many callers have to check, anyway, whether the value returned
by the command is or not empty (e.g. in GetExternalStoragePath). And it further
seems perfectly acceptable to return an empty string if the output was an empty
line (i.e. "\n", like "getprop ro.wifi.channels" does) without raising errors.

This behavior is also consistent with the idea that both an output of "hello"
and "hello\n" return the string "hello". Which again seems like a better
compromise than raising an exception if the command forgot to put the endline in
the output.

I guess that if a client *really* wants to distinguish between "" and "\n" in
the output, they might as well just call with single_line=False.

In the new patch I've updated the doc-string of this method, hopefully making it
more clear of what is the expected behavior.

+      elif len(output) == 1:
+        return output[0]
+      else:
+        msg = 'one line of output was expected, but got: %s'
+        raise device_errors.CommandFailedError(msg % output, str(self))
     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:
       process_name: A string containing the name of the process to kill.
@@ skipping 299 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):
+    """Returns the build type of the system (e.g. userdebug)."""
+    return self.GetProp('ro.build.type', cache=True)
+
+  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.
+      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_key = '_prop:' + property_name
+    if cache and cache_key in self._cache:
+      return self._cache[cache_key]
+    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_key in self._cache:
+        self._cache[cache_key] = 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.
+      check: A boolean indicating whether to check that the property was
+             successfully set on the device.
       timeout: timeout in seconds
       retries: number of retries
 
     Raises:
+      CommandFailedError if check is true and the property was not correctly
+        set on the device (e.g. because it is not rooted).
       CommandTimeoutError on timeout.
     """
-    self.old_interface.system_properties[property_name] = value
+    self.RunShellCommand(['setprop', property_name, value], check_return=True)
+    if property_name in self._cache:
+      del self._cache[property_name]
+    # TODO(perezju) remove the option and make the check mandatory, but using a
+    # single shell script to both set- and getprop.
+    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])