Move Python setup/tear down logic into Forwarder itself.

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 5bdb1cd4ce92b2598e9cff38295f19fd3950c572..9b45a367ca755fc3d97c5291b432ec1ba5bd085f 100644
--- a/build/android/pylib/forwarder.py
+++ b/build/android/pylib/forwarder.py
@@ -13,7 +13,7 @@ import android_commands
 import cmd_helper
 import constants
 
-from pylib import pexpect
+from pylib import valgrind_tools
 
 
 def _MakeBinaryPath(build_type, binary_name):
@@ -29,27 +29,11 @@ class Forwarder(object):
                             '/forwarder/device_forwarder')
   _LD_LIBRARY_PATH = 'LD_LIBRARY_PATH=%s' % _DEVICE_FORWARDER_FOLDER
 
-  def __init__(self, adb, build_type):
-    """Forwards TCP ports on the device back to the host.
+  _lock = threading.Lock()
+  _instance = None
 
-    Works like adb forward, but in reverse.
-
-    Args:
-      adb: Instance of AndroidCommands for talking to the device.
-      build_type: 'Release' or 'Debug'.
-    """
-    assert build_type in ('Release', 'Debug')
-    self._adb = adb
-    self._device_to_host_port_map = dict()
-    self._host_to_device_port_map = dict()
-    self._device_initialized = False
-    self._host_adb_control_port = 0
-    self._lock = threading.Lock()
-    self._host_forwarder_path = _MakeBinaryPath(build_type, 'host_forwarder')
-    self._device_forwarder_path_on_host = os.path.join(
-        cmd_helper.OutDirectory.get(), build_type, 'forwarder_dist')
-
-  def Run(self, port_pairs, tool):
+  @staticmethod
+  def Map(port_pairs, adb, build_type='Debug', tool=None):
     """Runs the forwarder.
 
     Args:
@@ -58,24 +42,28 @@ class Forwarder(object):
                  port will by dynamically assigned on the device. You can
                  get the number of the assigned port using the
                  DevicePortForHostPort method.
+      adb: An AndroidCommands instance.
       tool: Tool class to use to get wrapper, if necessary, for executing the
             forwarder (see valgrind_tools.py).
 
     Raises:
       Exception on failure to forward the port.
     """
-    with self._lock:
-      self._InitDeviceLocked(tool)
-      host_name = '127.0.0.1'
+    if not tool:
+      tool = valgrind_tools.CreateTool()
+    with Forwarder._lock:
+      instance = Forwarder._GetInstanceLocked(build_type, tool)
+      instance._InitDeviceOnceLocked(adb, tool)
+
       redirection_commands = [
-          ['--serial-id=' + self._adb.Adb().GetSerialNumber(), '--map',
+          ['--serial-id=' + adb.Adb().GetSerialNumber(), '--map',
            str(device), str(host)] for device, host in port_pairs]
       logging.info('Forwarding using commands: %s', redirection_commands)
 
       for redirection_command in redirection_commands:
         try:
           (exit_code, output) = cmd_helper.GetCmdStatusAndOutput(
-              [self._host_forwarder_path] + redirection_command)
+              [instance._host_forwarder_path] + redirection_command)
         except OSError as e:
           if e.errno == 2:
             raise Exception('Unable to start host forwarder. Make sure you have'
@@ -83,47 +71,84 @@ class Forwarder(object):
           else: raise
         if exit_code != 0:
           raise Exception('%s exited with %d:\n%s' % (
-              self._host_forwarder_path, exit_code, '\n'.join(output)))
+              instance._host_forwarder_path, exit_code, '\n'.join(output)))
         tokens = output.split(':')
         if len(tokens) != 2:
           raise Exception(('Unexpected host forwarder output "%s", ' +
                           'expected "device_port:host_port"') % output)
         device_port = int(tokens[0])
         host_port = int(tokens[1])
-        self._device_to_host_port_map[device_port] = host_port
-        self._host_to_device_port_map[host_port] = device_port
+        instance._device_to_host_port_map[device_port] = host_port
+        instance._host_to_device_port_map[host_port] = device_port
         logging.info('Forwarding device port: %d to host port: %d.',
                      device_port, host_port)
 
-  def _InitDeviceLocked(self, tool):
-    """Initializes the device forwarder process (only once)."""
-    if self._device_initialized:
+  @staticmethod
+  def UnmapDevicePort(device_port, adb):

[frankf, 2013/07/10 18:15:38]

Is it too dangerous to add a convenience method to unmap all the ports? It's a
common use case.

[Philippe, 2013/07/11 09:34:06]

Good idea.

+    """Unmaps a previously forwarded device port.
+
+    Args:
+      device_port: A previously forwarded port (through Map()).
+    """
+    with Forwarder._lock:
+      Forwarder._GetInstanceLocked(None, None)._UnmapDevicePortInternalLocked(

[frankf, 2013/07/10 18:15:38]

What happens if Unmap is called before Map?

[Philippe, 2013/07/11 09:34:06]

An error would be logged but no exception would be thrown. We used to throw an
exception but it proved to be too aggressive.

+          device_port, adb)
+
+  @staticmethod
+  def DevicePortForHostPort(host_port):
+    """Returns the device port that corresponds to a given host port."""
+    with Forwarder._lock:
+      return Forwarder._GetInstanceLocked(
+          None, None)._host_to_device_port_map.get(host_port)
+
+  @staticmethod
+  def _GetInstanceLocked(build_type, tool):
+    if not Forwarder._instance:
+      Forwarder._instance = Forwarder(build_type, tool)
+    return Forwarder._instance
+
+  def __init__(self, build_type, tool):

[frankf, 2013/07/10 18:15:38]

This is private and should be called once, maybe do:

assert not Forwarder._instance

[Philippe, 2013/07/11 09:34:06]

Good idea.

+    self._build_type = build_type
+    self._tool = tool
+    self._initialized_devices = set()
+    self._device_to_host_port_map = dict()
+    self._host_to_device_port_map = dict()
+    self._InitHostOnceLocked()
+    self._device_forwarder_path_on_host = os.path.join(
+        cmd_helper.OutDirectory.get(), self._build_type, 'forwarder_dist')
+
+  def _InitHostOnceLocked(self):
+    self._host_forwarder_path = _MakeBinaryPath(
+        self._build_type, 'host_forwarder')
+    if not os.path.exists(self._host_forwarder_path):
+      self._build_type = 'Release' if self._build_type == 'Debug' else 'Debug'

[frankf, 2013/07/10 18:15:38]

Why don't we know the definite path to the binary?

[Philippe, 2013/07/11 09:34:06]

In theory we do if the client provides the build_type. If he doesn't we default
to 'Debug' and fallback to 'Release' if the host_forwarder binary wasn't found.
A bunch of clients don't have the build type around. I found this a reasonable
compromise. What do you think?

+      self._host_forwarder_path = _MakeBinaryPath(
+          self._build_type, 'host_forwarder')
+      assert os.path.exists(
+          self._host_forwarder_path), 'Please build forwarder2'
+    self._KillHost()
+
+  def _InitDeviceOnceLocked(self, adb, tool):

[frankf, 2013/07/10 18:15:38]

Can you add brief docstrings for all methods in this module?

[Philippe, 2013/07/11 09:34:06]

Done.

+    device_serial = adb.Adb().GetSerialNumber()
+    if device_serial in self._initialized_devices:
       return
-    self._adb.PushIfNeeded(
+    Forwarder._KillDevice(adb, tool)
+    adb.PushIfNeeded(
         self._device_forwarder_path_on_host,
         Forwarder._DEVICE_FORWARDER_FOLDER)
-    (exit_code, output) = self._adb.GetShellCommandStatusAndOutput(
+    (exit_code, output) = adb.GetShellCommandStatusAndOutput(
         '%s %s %s' % (Forwarder._LD_LIBRARY_PATH, tool.GetUtilWrapper(),
                       Forwarder._DEVICE_FORWARDER_PATH))
     if exit_code != 0:
       raise Exception(
           'Failed to start device forwarder:\n%s' % '\n'.join(output))
-    self._device_initialized = True
-
-  def UnmapDevicePort(self, device_port):
-    """Unmaps a previously forwarded device port.
-
-    Args:
-      device_port: A previously forwarded port (through Run()).
-    """
-    with self._lock:
-      self._UnmapDevicePortInternalLocked(device_port)
+    self._initialized_devices.add(device_serial)
 
-  def _UnmapDevicePortInternalLocked(self, device_port):
+  def _UnmapDevicePortInternalLocked(self, device_port, adb):

[Philippe, 2013/07/11 09:34:06]

FYI, I inlined this method in the latest patch set since it now has a single
caller.

     if not device_port in self._device_to_host_port_map:
       return
     redirection_command = [
-        '--serial-id=' + self._adb.Adb().GetSerialNumber(), '--unmap',
+        '--serial-id=' + adb.Adb().GetSerialNumber(), '--unmap',
         str(device_port)]
     (exit_code, output) = cmd_helper.GetCmdStatusAndOutput(
         [self._host_forwarder_path] + redirection_command)
@@ -134,30 +159,20 @@ class Forwarder(object):
     del self._device_to_host_port_map[device_port]
     del self._host_to_device_port_map[host_port]
 
-  @staticmethod
-  def KillHost(build_type='Debug'):
-    """Kills the forwarder process running on the host.
-
-    Args:
-      build_type: 'Release' or 'Debug' (default='Debug')
-    """
+  def _KillHost(self):
+    """Kills the forwarder process running on the host."""
     logging.info('Killing host_forwarder.')
-    host_forwarder_path = _MakeBinaryPath(build_type, 'host_forwarder')
-    if not os.path.exists(host_forwarder_path):
-      host_forwarder_path = _MakeBinaryPath(
-          'Release' if build_type == 'Debug' else 'Debug', 'host_forwarder')
-    assert os.path.exists(host_forwarder_path), 'Please build forwarder2'
     (exit_code, output) = cmd_helper.GetCmdStatusAndOutput(
-        [host_forwarder_path, '--kill-server'])
+        [self._host_forwarder_path, '--kill-server'])
     if exit_code != 0:
       (exit_code, output) = cmd_helper.GetCmdStatusAndOutput(
-          ['pkill', 'host_forwarder'])
+          ['pkill', '-9', 'host_forwarder'])
       if exit_code != 0:
         raise Exception('%s exited with %d:\n%s' % (
-              host_forwarder_path, exit_code, '\n'.join(output)))
+              self._host_forwarder_path, exit_code, '\n'.join(output)))
 
   @staticmethod
-  def KillDevice(adb, tool):
+  def _KillDevice(adb, tool):
     """Kills the forwarder process running on the device.
 
     Args:
@@ -180,14 +195,3 @@ class Forwarder(object):
       pids = adb.ExtractPid('device_forwarder')
       if pids:
         raise Exception('Timed out while killing device_forwarder')
-
-  def DevicePortForHostPort(self, host_port):
-    """Returns the device port that corresponds to a given host port."""
-    with self._lock:
-      return self._host_to_device_port_map.get(host_port)
-
-  def Close(self):
-    """Releases the previously forwarded ports."""
-    with self._lock:
-      for device_port in self._device_to_host_port_map.copy():
-        self._UnmapDevicePortInternalLocked(device_port)