Cleanup common port forwarding code (#7142)

This moves the various copies of port forwarding code in the Device subclasses into the ProtocolDiscovery class.

* move port forwarding to a common location
* throw exception if protocol Uri is not discovered or port forwarding fails
* cancel discovery protocol subscriptions on iOS launches (wasn't happening before)
* fix iOS port forwarding to match other implementations
* add tests

packages/flutter_tools/lib/src/android/android_device.dart

@@ -8,7 +8,6 @@ import 'dart:io';
 
 import '../android/android_sdk.dart';
 import '../application_package.dart';
-import '../base/common.dart';
 import '../base/os.dart';
 import '../base/logger.dart';
 import '../base/process.dart';
@@ -261,17 +260,6 @@ class AndroidDevice extends Device {
     return true;
   }
 
-  Future<int> _forwardPort(int hostPort, int devicePort) async {
-    try {
-      hostPort = await portForwarder.forward(devicePort, hostPort: hostPort);
-      printTrace('Forwarded host port $hostPort to device port $devicePort');
-      return hostPort;
-    } catch (e) {
-      throw new ToolExit(
-          'Unable to forward host port $hostPort to device port $devicePort: $e');
-    }
-  }
-
   @override
   Future<LaunchResult> startApp(
     ApplicationPackage package,
@@ -315,10 +303,13 @@ class AndroidDevice extends Device {
     ProtocolDiscovery observatoryDiscovery;
     ProtocolDiscovery diagnosticDiscovery;
 
-    DeviceLogReader logReader = getLogReader();
     if (debuggingOptions.debuggingEnabled) {
-      observatoryDiscovery = new ProtocolDiscovery(logReader, ProtocolDiscovery.kObservatoryService);
-      diagnosticDiscovery = new ProtocolDiscovery(logReader, ProtocolDiscovery.kDiagnosticService);
+      // TODO(devoncarew): Remember the forwarding information (so we can later remove the
+      // port forwarding).
+      observatoryDiscovery = new ProtocolDiscovery.observatory(
+        getLogReader(), portForwarder: portForwarder, hostPort: debuggingOptions.observatoryPort);
+      diagnosticDiscovery = new ProtocolDiscovery.diagnosticService(
+        getLogReader(), portForwarder: portForwarder, hostPort: debuggingOptions.diagnosticPort);
     }
 
     List<String> cmd;
@@ -351,56 +342,37 @@ class AndroidDevice extends Device {
 
     if (!debuggingOptions.debuggingEnabled) {
       return new LaunchResult.succeeded();
-    } else {
-      // Wait for the service protocol port here. This will complete once the
-      // device has printed "Observatory is listening on...".
-      printTrace('Waiting for observatory port to be available...');
+    }
 
-      // TODO(danrubel): The iOS device class does something similar to this code below.
-      // The various Device subclasses should be refactored and common code moved into the superclass.
-      try {
-        Uri observatoryDeviceUri, diagnosticDeviceUri;
-
-        if (debuggingOptions.buildMode == BuildMode.debug) {
-          Future<List<Uri>> scrapeServiceUris = Future.wait(
-              <Future<Uri>>[observatoryDiscovery.nextUri(), diagnosticDiscovery.nextUri()]
-          );
-          List<Uri> deviceUris = await scrapeServiceUris.timeout(new Duration(seconds: 20));
-          observatoryDeviceUri = deviceUris[0];
-          diagnosticDeviceUri = deviceUris[1];
-        } else if (debuggingOptions.buildMode == BuildMode.profile) {
-          observatoryDeviceUri = await observatoryDiscovery.nextUri().timeout(new Duration(seconds: 20));
-        }
+    // Wait for the service protocol port here. This will complete once the
+    // device has printed "Observatory is listening on...".
+    printTrace('Waiting for observatory port to be available...');
 
-        printTrace('Observatory Uri on device: $observatoryDeviceUri');
-        int observatoryLocalPort = await debuggingOptions.findBestObservatoryPort();
-        // TODO(devoncarew): Remember the forwarding information (so we can later remove the
-        // port forwarding).
-        observatoryLocalPort = await _forwardPort(observatoryLocalPort, observatoryDeviceUri.port);
-        Uri observatoryLocalUri = observatoryDeviceUri.replace(port: observatoryLocalPort);
-
-        Uri diagnosticLocalUri;
-        if (diagnosticDeviceUri != null) {
-          printTrace('Diagnostic Server Uri on device: $diagnosticDeviceUri');
-          int diagnosticLocalPort = await debuggingOptions.findBestDiagnosticPort();
-          diagnosticLocalPort = await _forwardPort(diagnosticLocalPort, diagnosticDeviceUri.port);
-          diagnosticLocalUri = diagnosticDeviceUri.replace(port: diagnosticLocalPort);
-        }
+    // TODO(danrubel) Waiting for observatory and diagnostic services
+    // can be made common across all devices.
+    try {
+      Uri observatoryUri, diagnosticUri;
 
-        return new LaunchResult.succeeded(
-            observatoryUri: observatoryLocalUri,
-            diagnosticUri: diagnosticLocalUri,
+      if (debuggingOptions.buildMode == BuildMode.debug) {
+        List<Uri> deviceUris = await Future.wait(
+            <Future<Uri>>[observatoryDiscovery.nextUri(), diagnosticDiscovery.nextUri()]
         );
-      } catch (error) {
-        if (error is TimeoutException)
-          printError('Timed out while waiting for a debug connection.');
-        else
-          printError('Error waiting for a debug connection: $error');
-        return new LaunchResult.failed();
-      } finally {
-        observatoryDiscovery.cancel();
-        diagnosticDiscovery.cancel();
+        observatoryUri = deviceUris[0];
+        diagnosticUri = deviceUris[1];
+      } else if (debuggingOptions.buildMode == BuildMode.profile) {
+        observatoryUri = await observatoryDiscovery.nextUri();
       }
+
+      return new LaunchResult.succeeded(
+          observatoryUri: observatoryUri,
+          diagnosticUri: diagnosticUri,
+      );
+    } catch (error) {
+      printError('Error waiting for a debug connection: $error');
+      return new LaunchResult.failed();
+    } finally {
+      observatoryDiscovery.cancel();
+      diagnosticDiscovery.cancel();
     }
   }
 

packages/flutter_tools/lib/src/device.dart

@@ -175,6 +175,22 @@ abstract class Device {
   /// Get the port forwarder for this device.
   DevicePortForwarder get portForwarder;
 
+  Future<int> forwardPort(int devicePort, {int hostPort}) async {
+    try {
+      hostPort = await portForwarder
+          .forward(devicePort, hostPort: hostPort)
+          .timeout(const Duration(seconds: 60), onTimeout: () {
+            throw new ToolExit(
+                'Timeout while atempting to foward device port $devicePort');
+          });
+      printTrace('Forwarded host port $hostPort to device port $devicePort');
+      return hostPort;
+    } catch (e) {
+      throw new ToolExit(
+          'Unable to forward host port $hostPort to device port $devicePort: $e');
+    }
+  }
+
   /// Clear the device's logs.
   void clearLogs();
 

packages/flutter_tools/lib/src/ios/devices.dart

@@ -260,18 +260,15 @@ class IOSDevice extends Device {
 
       // TODO(danrubel): The Android device class does something similar to this code below.
       // The various Device subclasses should be refactored and common code moved into the superclass.
-      Future<Uri> forwardObsUri = _acquireServiceUri(
-        app,
-        ProtocolDiscovery.kObservatoryService,
-        debuggingOptions.observatoryPort,
-      );
+      ProtocolDiscovery observatoryDiscovery = new ProtocolDiscovery.observatory(
+        getLogReader(app: app), portForwarder: portForwarder, hostPort: debuggingOptions.observatoryPort);
+      ProtocolDiscovery diagnosticDiscovery = new ProtocolDiscovery.diagnosticService(
+        getLogReader(app: app), portForwarder: portForwarder, hostPort: debuggingOptions.diagnosticPort);
+
+      Future<Uri> forwardObsUri = observatoryDiscovery.nextUri();
       Future<Uri> forwardDiagUri;
       if (debuggingOptions.buildMode == BuildMode.debug) {
-        forwardDiagUri = _acquireServiceUri(
-          app,
-          ProtocolDiscovery.kDiagnosticService,
-          debuggingOptions.diagnosticPort,
-        );
+        forwardDiagUri = diagnosticDiscovery.nextUri();
       } else {
         forwardDiagUri = new Future<Uri>.value(null);
       }
@@ -287,18 +284,12 @@ class IOSDevice extends Device {
         }
 
         printTrace("Application launched on the device. Attempting to forward ports.");
-        return await Future.wait(<Future<Uri>>[forwardObsUri, forwardDiagUri])
-            .timeout(
-                kPortForwardTimeout,
-                onTimeout: () {
-                  throw new TimeoutException('Timeout while waiting to acquire and forward ports.');
-                },
-            );
+        return await Future.wait(<Future<Uri>>[forwardObsUri, forwardDiagUri]);
+      }).whenComplete(() {
+        observatoryDiscovery.cancel();
+        diagnosticDiscovery.cancel();
       });
 
-      printTrace("Observatory Uri on device: ${uris[0]}");
-      printTrace("Diagnostic Server Uri on device: ${uris[1]}");
-
       localObsUri = uris[0];
       localDiagUri = uris[1];
     }
@@ -314,48 +305,6 @@ class IOSDevice extends Device {
     return new LaunchResult.succeeded(observatoryUri: localObsUri, diagnosticUri: localDiagUri);
   }
 
-  Future<Uri> _acquireServiceUri(
-      ApplicationPackage app,
-      String serviceName,
-      int localPort) async {
-    Duration stepTimeout = const Duration(seconds: 60);
-
-    Future<Uri> remote = new ProtocolDiscovery(getLogReader(app: app), serviceName).nextUri();
-
-    Uri remoteUri = await remote.timeout(stepTimeout,
-        onTimeout: () {
-      printTrace("Timeout while attempting to retrieve remote Uri for $serviceName");
-      return null;
-    });
-
-    if (remoteUri == null) {
-      printTrace("Could not read Uri on device for $serviceName");
-      return null;
-    }
-
-    if ((localPort == null) || (localPort == 0)) {
-      localPort = await findAvailablePort();
-      printTrace("Auto selected local port to $localPort");
-    }
-
-    int forwardResult = await portForwarder
-      .forward(remoteUri.port, hostPort: localPort)
-      .timeout(stepTimeout, onTimeout: () {
-        printTrace("Timeout while atempting to foward port for $serviceName");
-        return null;
-      });
-
-    if (forwardResult == null) {
-      printTrace("Could not foward remote $serviceName port $remoteUri to local port $localPort");
-      return null;
-    }
-
-    Uri forwardUri = remoteUri.replace(port: forwardResult);
-
-    printStatus('$serviceName listening on $forwardUri');
-    return forwardUri;
-  }
-
   @override
   Future<bool> stopApp(ApplicationPackage app) async {
     // Currently we don't have a way to stop an app running on iOS.
@@ -509,7 +458,7 @@ class _IOSDevicePortForwarder extends DevicePortForwarder {
 
     _forwardedPorts.add(forwardedPort);
 
-    return 1;
+    return hostPort;
   }
 
   @override

packages/flutter_tools/lib/src/ios/simulators.dart

@@ -430,12 +430,6 @@ class IOSSimulator extends Device {
         return new LaunchResult.failed();
     }
 
-    ProtocolDiscovery observatoryDiscovery;
-
-    if (debuggingOptions.debuggingEnabled)
-      observatoryDiscovery = new ProtocolDiscovery(getLogReader(app: app),
-                                                   ProtocolDiscovery.kObservatoryService);
-
     // Prepare launch arguments.
     List<String> args = <String>["--enable-dart-profiling"];
 
@@ -467,27 +461,21 @@ class IOSSimulator extends Device {
 
     if (!debuggingOptions.debuggingEnabled) {
       return new LaunchResult.succeeded();
-    } else {
-      // Wait for the service protocol port here. This will complete once the
-      // device has printed "Observatory is listening on..."
-      printTrace('Waiting for observatory port to be available...');
+    }
 
-      try {
-        Uri deviceUri = await observatoryDiscovery
-          .nextUri()
-          .timeout(new Duration(seconds: 20));
-        printTrace('Observatory Uri on simulator: $deviceUri');
-        printStatus('Observatory listening on $deviceUri');
-        return new LaunchResult.succeeded(observatoryUri: deviceUri);
-      } catch (error) {
-        if (error is TimeoutException)
-          printError('Timed out while waiting for a debug connection.');
-        else
-          printError('Error waiting for a debug connection: $error');
-        return new LaunchResult.failed();
-      } finally {
-        observatoryDiscovery.cancel();
-      }
+    // Wait for the service protocol port here. This will complete once the
+    // device has printed "Observatory is listening on..."
+    printTrace('Waiting for observatory port to be available...');
+
+    ProtocolDiscovery observatoryDiscovery = new ProtocolDiscovery.observatory(getLogReader(app: app));
+    try {
+      Uri deviceUri = await observatoryDiscovery.nextUri();
+      return new LaunchResult.succeeded(observatoryUri: deviceUri);
+    } catch (error) {
+      printError('Error waiting for a debug connection: $error');
+      return new LaunchResult.failed();
+    } finally {
+      observatoryDiscovery.cancel();
     }
   }
 

packages/flutter_tools/lib/src/protocol_discovery.dart

@@ -4,29 +4,75 @@
 
 import 'dart:async';
 
+import 'base/common.dart';
+import 'base/os.dart';
 import 'device.dart';
+import 'globals.dart';
 
-/// Discover service protocol ports on devices.
+/// Discover service protocol on a device
+/// and forward the service protocol device port to the host.
 class ProtocolDiscovery {
   /// [logReader] - a [DeviceLogReader] to look for service messages in.
-  ProtocolDiscovery(DeviceLogReader logReader, String serviceName)
+  ProtocolDiscovery(DeviceLogReader logReader, String serviceName,
+      {this.portForwarder, this.hostPort, this.defaultHostPort})
       : _logReader = logReader, _serviceName = serviceName {
     assert(_logReader != null);
     _subscription = _logReader.logLines.listen(_onLine);
+    assert(portForwarder == null || defaultHostPort != null);
   }
 
+  factory ProtocolDiscovery.observatory(DeviceLogReader logReader,
+          {DevicePortForwarder portForwarder, int hostPort}) =>
+      new ProtocolDiscovery(logReader, kObservatoryService,
+          portForwarder: portForwarder,
+          hostPort: hostPort,
+          defaultHostPort: kDefaultObservatoryPort);
+
+  factory ProtocolDiscovery.diagnosticService(DeviceLogReader logReader,
+          {DevicePortForwarder portForwarder, int hostPort}) =>
+      new ProtocolDiscovery(logReader, kDiagnosticService,
+          portForwarder: portForwarder,
+          hostPort: hostPort,
+          defaultHostPort: kDefaultDiagnosticPort);
+
   static const String kObservatoryService = 'Observatory';
   static const String kDiagnosticService = 'Diagnostic server';
 
   final DeviceLogReader _logReader;
   final String _serviceName;
+  final DevicePortForwarder portForwarder;
+  int hostPort;
+  final int defaultHostPort;
 
   Completer<Uri> _completer = new Completer<Uri>();
   StreamSubscription<String> _subscription;
 
   /// The [Future] returned by this function will complete when the next service
   /// Uri is found.
-  Future<Uri> nextUri() => _completer.future;
+  Future<Uri> nextUri() async {
+    Uri deviceUri = await _completer.future.timeout(
+      const Duration(seconds: 60), onTimeout: () {
+        throwToolExit('Timeout while attempting to retrieve Uri for $_serviceName');
+      }
+    );
+    printTrace('$_serviceName Uri on device: $deviceUri');
+    Uri hostUri;
+    if (portForwarder != null) {
+      int devicePort = deviceUri.port;
+      hostPort ??= await findPreferredPort(defaultHostPort);
+      hostPort = await portForwarder
+          .forward(devicePort, hostPort: hostPort)
+          .timeout(const Duration(seconds: 60), onTimeout: () {
+            throwToolExit('Timeout while atempting to foward device port $devicePort');
+          });
+      printTrace('Forwarded host port $hostPort to device port $devicePort');
+      hostUri = deviceUri.replace(port: hostPort);
+    } else {
+      hostUri = deviceUri;
+    }
+    printStatus('$_serviceName listening on $hostUri');
+    return hostUri;
+  }
 
   void cancel() {
     _subscription.cancel();

packages/flutter_tools/test/protocol_discovery_test.dart

@@ -4,14 +4,16 @@
 
 import 'dart:async';
 
+import 'package:flutter_tools/src/device.dart';
 import 'package:flutter_tools/src/protocol_discovery.dart';
 import 'package:test/test.dart';
 
+import 'src/context.dart';
 import 'src/mocks.dart';
 
 void main() {
-  group('service_protocol', () {
-    test('Discovery Heartbeat', () async {
+  group('service_protocol discovery', () {
+    testUsingContext('no port forwarding', () async {
       MockDeviceLogReader logReader = new MockDeviceLogReader();
       ProtocolDiscovery discoverer =
           new ProtocolDiscovery(logReader, ProtocolDiscovery.kObservatoryService);
@@ -68,5 +70,60 @@ void main() {
       discoverer.cancel();
       logReader.dispose();
     });
+
+    testUsingContext('port forwarding - default port', () async {
+      MockDeviceLogReader logReader = new MockDeviceLogReader();
+      ProtocolDiscovery discoverer = new ProtocolDiscovery(
+          logReader,
+          ProtocolDiscovery.kObservatoryService,
+          portForwarder: new MockPortForwarder(99),
+          defaultHostPort: 54777);
+
+      // Get next port future.
+      Future<Uri> nextUri = discoverer.nextUri();
+      logReader.addLine('I/flutter : Observatory listening on http://somehost:54804/PTwjm8Ii8qg=/');
+      Uri uri = await nextUri;
+      expect(uri.port, 54777);
+      expect('$uri', 'http://somehost:54777/PTwjm8Ii8qg=/');
+
+      discoverer.cancel();
+      logReader.dispose();
+    });
+
+    testUsingContext('port forwarding - specified port', () async {
+      MockDeviceLogReader logReader = new MockDeviceLogReader();
+      ProtocolDiscovery discoverer = new ProtocolDiscovery(
+          logReader,
+          ProtocolDiscovery.kObservatoryService,
+          portForwarder: new MockPortForwarder(99),
+          hostPort: 1243,
+          defaultHostPort: 192);
+
+      // Get next port future.
+      Future<Uri> nextUri = discoverer.nextUri();
+      logReader.addLine('I/flutter : Observatory listening on http://somehost:54804/PTwjm8Ii8qg=/');
+      Uri uri = await nextUri;
+      expect(uri.port, 1243);
+      expect('$uri', 'http://somehost:1243/PTwjm8Ii8qg=/');
+
+      discoverer.cancel();
+      logReader.dispose();
+    });
   });
 }
+
+class MockPortForwarder extends DevicePortForwarder {
+  final int availablePort;
+  MockPortForwarder([this.availablePort]);
+
+  @override
+  Future<int> forward(int devicePort, {int hostPort}) async => hostPort ?? availablePort;
+
+  @override
+  List<ForwardedPort> get forwardedPorts => throw 'not implemented';
+
+  @override
+  Future<Null> unforward(ForwardedPort forwardedPort) {
+    throw 'not implemented';
+  }
+}