Use the new forwarder2's Daemon implementation in device_forwarder.

build/android/pylib/forwarder.py

Index: build/android/pylib/forwarder.py
diff --git a/build/android/pylib/forwarder.py b/build/android/pylib/forwarder.py
index a0dc5f2fcc1d7ff59696d988968eeb85d614570b..a55ae5d71a69c3850ae2c7b4a234040feb870872 100644
--- a/build/android/pylib/forwarder.py
+++ b/build/android/pylib/forwarder.py
@@ -23,12 +23,12 @@ def _MakeBinaryPath(build_type, binary_name):
 class Forwarder(object):
   """Class to manage port forwards from the device to the host."""
 
-  _DEVICE_FORWARDER_PATH = constants.TEST_EXECUTABLE_DIR + '/device_forwarder'
-
   # Unix Abstract socket path:
   _DEVICE_ADB_CONTROL_PORT = 'chrome_device_forwarder'
   _TIMEOUT_SECS = 30
 
+  _DEVICE_FORWARDER_PATH = constants.TEST_EXECUTABLE_DIR + '/device_forwarder'
+
   def __init__(self, adb, build_type):
     """Forwards TCP ports on the device back to the host.
 
@@ -66,8 +66,8 @@ class Forwarder(object):
     host_adb_control_port = ports.AllocateTestServerPort()
     if not host_adb_control_port:
       raise Exception('Failed to allocate a TCP port in the host machine.')
-    self._adb.PushIfNeeded(self._device_forwarder_path,
-                           Forwarder._DEVICE_FORWARDER_PATH)
+    self._adb.PushIfNeeded(
+        self._device_forwarder_path, Forwarder._DEVICE_FORWARDER_PATH)
     redirection_commands = [
         '%d:%d:%d:%s' % (host_adb_control_port, device, host,
                          host_name) for device, host in port_pairs]
@@ -80,48 +80,19 @@ class Forwarder(object):
          'localabstract:%s' % Forwarder._DEVICE_ADB_CONTROL_PORT]) != 0:
       raise Exception('Error while running adb forward.')
 
-    if not self._adb.ExtractPid('device_forwarder'):
-      # TODO(pliard): Get rid of pexpect here and make device_forwarder a daemon
-      # with a blocking CLI process that exits with a proper exit code and not
-      # while the daemon is still setting up. This would be similar to how
-      # host_forwarder works.
-      self._device_process = pexpect.spawn(
-          'adb', ['-s',
-                  self._adb._adb.GetSerialNumber(),
-                  'shell',
-                  '%s %s -D --adb_sock=%s' % (
-                      tool.GetUtilWrapper(),
-                      Forwarder._DEVICE_FORWARDER_PATH,
-                      Forwarder._DEVICE_ADB_CONTROL_PORT)])
-      device_success_re = re.compile('Starting Device Forwarder.')
-      device_failure_re = re.compile('.*:ERROR:(.*)')
-      index = self._device_process.expect([device_success_re,
-                                           device_failure_re,
-                                           pexpect.EOF,
-                                           pexpect.TIMEOUT],
-                                           Forwarder._TIMEOUT_SECS)
-      if index == 1:
-        error_msg = str(self._device_process.match.group(1))
-        logging.error(self._device_process.before)
-        self._device_process.close()
-        raise Exception('Failed to start Device Forwarder with Error: %s' %
-                        error_msg)
-      elif index == 2:
-        logging.error(self._device_process.before)
-        self._device_process.close()
-        raise Exception(
-            'Unexpected EOF while trying to start Device Forwarder.')
-      elif index == 3:
-        logging.error(self._device_process.before)
-        self._device_process.close()
-        raise Exception('Timeout while trying start Device Forwarder')
+    (exit_code, output) = self._adb.GetShellCommandStatusAndOutput(
+        '%s %s' % (Forwarder._DEVICE_FORWARDER_PATH,
+                   Forwarder._DEVICE_ADB_CONTROL_PORT))
+    if exit_code != 0:
+      raise Exception('Failed to start device forwarder:\n%s' % (

[bulach, 2012/11/15 02:08:19]

nit: no need for the parens at EOL

[Philippe, 2012/11/15 09:56:20]

Thanks for the tip, I thought it was needed to break the line.

[bulach, 2012/11/15 18:28:10]

yeah, the parens is needed to break line :) however, Exception( had one already,
that is, the interpreter keeps reading the following lines until the first
parens is closed..

[Philippe, 2012/11/19 10:22:14]

Ah indeed, I should have noticed it :)

+                      '\n'.join(output)))
 
     for redirection_command in redirection_commands:
       (exit_code, output) = cmd_helper.GetCmdStatusAndOutput(
           [self._host_forwarder_path, redirection_command])
       if exit_code != 0:
-        raise Exception('%s exited with %d: %s' % (self._host_forwarder_path,
-                                                   exit_code, output))
+        raise Exception('%s exited with %d:\n%s' % (
+            self._host_forwarder_path, exit_code, '\n'.join(output)))
       tokens = output.split(':')
       if len(tokens) != 2:
         raise Exception('Unexpected host forwarder output "%s", ' +
@@ -138,12 +109,20 @@ class Forwarder(object):
     (exit_code, output) = cmd_helper.GetCmdStatusAndOutput(
         [host_forwarder_path, 'kill-server'])
     if exit_code != 0:
-      raise Exception('%s exited with %d: %s' % (host_forwarder_path,
-                                                 exit_code, output))
+      raise Exception('%s exited with %d:\n%s' % (host_forwarder_path,
+                                                  exit_code, '\n'.join(output)))
 
   @staticmethod
-  def KillDevice(adb):
+  def KillDevice(adb, build_type):
     logging.info('Killing device_forwarder.')
+    adb.PushIfNeeded(
+        _MakeBinaryPath(build_type, 'device_forwarder'),
+        Forwarder._DEVICE_FORWARDER_PATH)

[bulach, 2012/11/15 02:08:19]

why is it needed? I'd assume that on kill it's already in the device?

[Philippe, 2012/11/15 09:56:20]

I do a very early kill in some places in case the tear down  for the previous
run didn't happen for some reason. I found this bug when I tried this change on
a brand new device.

[bulach, 2012/11/15 18:28:10]

ahn, I see...
I think this is a real corner case, so it's probably simpler to get around with
adb.FileExistsOnDevice / early return, wdyt?

[Philippe, 2012/11/19 10:22:14]

Indeed.

+    (exit_code, output) = adb.GetShellCommandStatusAndOutput(
+        '%s kill-server' % Forwarder._DEVICE_FORWARDER_PATH)
+    # TODO(pliard): Remove the following call to KillAllBlocking() when we are

[bulach, 2012/11/15 02:08:19]

isn't the new forwarder pushed on Run? not sure how this scenario would
happen...

[Philippe, 2012/11/15 09:56:20]

Your question here seems similar to the previous one. In case you are wondering
why this TODO is here:
If there is still an old version of device_forwarder running on the device
(which doesn't support the 'kill-server' command) then pushing the new version
and doing a '$ device_forwarder kill-server' won't be enough. Daemon::Kill()
fetches the daemon's PID (used to do the kill) by looking at a file lock
(acquired by the daemon when it's spawned). The old version of device_forwarder
didn't use this whole mechanism of file locking so a kill-server wouldn't
actually kill the (old) device_forwarder.

[bulach, 2012/11/15 18:28:10]

that makes sense, thanks for the detailed information!

+    # sure that the old version of device_forwarder (not supporting
+    # 'kill-server') is not running on the bots anymore.
     timeout_sec = 5
     processes_killed = adb.KillAllBlocking('device_forwarder', timeout_sec)
     if not processes_killed: