Commit 6aa01dbd authored by tonyg's avatar tonyg Committed by Commit bot

[Telemetry] Properly chose an available device port.

Previously, we'd start WPR on an available host port and then just
assume that the same port was free on the device -- oops.

Now, we pass 0 to the forwarder for the device port so that it chooses
an available port, and then read back the port that it chose to use
for the mapping.

I believe this fixes a major case of device forwarder flake.

BUG=421599
TBR=slamm@chromium.org

Review URL: https://codereview.chromium.org/640133002

Cr-Commit-Position: refs/heads/master@{#299319}
parent 2411ba37
...@@ -33,7 +33,9 @@ class Forwarder(object): ...@@ -33,7 +33,9 @@ class Forwarder(object):
def __init__(self, port_pairs): def __init__(self, port_pairs):
assert port_pairs.http, 'HTTP port mapping is required.' assert port_pairs.http, 'HTTP port mapping is required.'
self._port_pairs = port_pairs self._port_pairs = PortPairs(*[
PortPair(p.local_port, p.remote_port or p.local_port)
if p else None for p in port_pairs])
@property @property
def host_port(self): def host_port(self):
...@@ -43,6 +45,10 @@ class Forwarder(object): ...@@ -43,6 +45,10 @@ class Forwarder(object):
def host_ip(self): def host_ip(self):
return '127.0.0.1' return '127.0.0.1'
@property
def port_pairs(self):
return self._port_pairs
@property @property
def url(self): def url(self):
assert self.host_ip and self.host_port assert self.host_ip and self.host_port
......
...@@ -54,13 +54,19 @@ class AndroidForwarder(forwarders.Forwarder): ...@@ -54,13 +54,19 @@ class AndroidForwarder(forwarders.Forwarder):
def __init__(self, adb, port_pairs): def __init__(self, adb, port_pairs):
super(AndroidForwarder, self).__init__(port_pairs) super(AndroidForwarder, self).__init__(port_pairs)
self._device = adb.device() self._device = adb.device()
forwarder.Forwarder.Map([p for p in port_pairs if p], self._device) forwarder.Forwarder.Map([(p.remote_port, p.local_port)
for p in port_pairs if p], self._device)
self._port_pairs = forwarders.PortPairs(*[
forwarders.PortPair(
p.local_port,
forwarder.Forwarder.DevicePortForHostPort(p.local_port))
if p else None for p in port_pairs])
# TODO(tonyg): Verify that each port can connect to host. # TODO(tonyg): Verify that each port can connect to host.
def Close(self): def Close(self):
for port_pair in self._port_pairs: for port_pair in self._port_pairs:
if port_pair: if port_pair:
forwarder.Forwarder.UnmapDevicePort(port_pair.local_port, self._device) forwarder.Forwarder.UnmapDevicePort(port_pair.remote_port, self._device)
super(AndroidForwarder, self).Close() super(AndroidForwarder, self).Close()
......
...@@ -47,10 +47,10 @@ class ReplayServer(object): ...@@ -47,10 +47,10 @@ class ReplayServer(object):
# Assign the forwarder port pairs back to the browser_backend. # Assign the forwarder port pairs back to the browser_backend.
# The port pairs are used to set up the application. # The port pairs are used to set up the application.
# The chrome_browser_backend uses the remote ports to set browser flags. # The chrome_browser_backend uses the remote ports to set browser flags.
browser_backend.wpr_port_pairs = self._ForwarderPortPairs( port_pairs = self._ForwarderPortPairs(
started_ports, browser_backend.wpr_port_pairs) started_ports, browser_backend.wpr_port_pairs)
self._forwarder = browser_backend.forwarder_factory.Create( self._forwarder = browser_backend.forwarder_factory.Create(port_pairs)
browser_backend.wpr_port_pairs) browser_backend.wpr_port_pairs = self._forwarder.port_pairs
@staticmethod @staticmethod
def _ForwarderPortPairs(started_ports, wpr_port_pairs): def _ForwarderPortPairs(started_ports, wpr_port_pairs):
...@@ -70,8 +70,8 @@ class ReplayServer(object): ...@@ -70,8 +70,8 @@ class ReplayServer(object):
a forwarders.PortPairs instance used to create the forwarder. a forwarders.PortPairs instance used to create the forwarder.
""" """
local_http_port, local_https_port, local_dns_port = started_ports local_http_port, local_https_port, local_dns_port = started_ports
remote_http_port = wpr_port_pairs.http.remote_port or local_http_port remote_http_port = wpr_port_pairs.http.remote_port
remote_https_port = wpr_port_pairs.https.remote_port or local_https_port remote_https_port = wpr_port_pairs.https.remote_port
http_port_pair = forwarders.PortPair(local_http_port, remote_http_port) http_port_pair = forwarders.PortPair(local_http_port, remote_http_port)
https_port_pair = forwarders.PortPair(local_https_port, remote_https_port) https_port_pair = forwarders.PortPair(local_https_port, remote_https_port)
if wpr_port_pairs.dns is None: if wpr_port_pairs.dns is None:
......
...@@ -13,8 +13,8 @@ class ForwarderPortPairsTest(unittest.TestCase): ...@@ -13,8 +13,8 @@ class ForwarderPortPairsTest(unittest.TestCase):
def testNoRemotePortsGivesLocalToLocal(self): def testNoRemotePortsGivesLocalToLocal(self):
started_ports = (8080, 8443, None) started_ports = (8080, 8443, None)
wpr_port_pairs = forwarders.PortPairs( wpr_port_pairs = forwarders.PortPairs(
http=forwarders.PortPair(0, 0), http=forwarders.PortPair(0, 8080),
https=forwarders.PortPair(0, 0), https=forwarders.PortPair(0, 8443),
dns=None) dns=None)
expected_port_pairs = forwarders.PortPairs( expected_port_pairs = forwarders.PortPairs(
http=forwarders.PortPair(8080, 8080), http=forwarders.PortPair(8080, 8080),
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment