[flutter_tool] Observatory connection error handling cleanup (#38353)

packages/flutter_tools/lib/src/commands/attach.dart

@@ -243,6 +243,8 @@ class AttachCommand extends FlutterCommand {
           // Determine ipv6 status from the scanned logs.
           usesIpv6 = observatoryDiscovery.ipv6;
           printStatus('Done.'); // FYI, this message is used as a sentinel in tests.
+        } catch (error) {
+          throwToolExit('Failed to establish a debug connection with ${device.name}: $error');
         } finally {
           await observatoryDiscovery?.cancel();
         }

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

@@ -152,9 +152,9 @@ class IOSDevice extends Device {
   @override
   final String name;
 
-  Map<ApplicationPackage, _IOSDeviceLogReader> _logReaders;
+  Map<ApplicationPackage, DeviceLogReader> _logReaders;
 
-  _IOSDevicePortForwarder _portForwarder;
+  DevicePortForwarder _portForwarder;
 
   @override
   Future<bool> get isLocalEmulator async => false;
@@ -342,65 +342,55 @@ class IOSDevice extends Device {
     if (platformArgs['trace-startup'] ?? false)
       launchArguments.add('--trace-startup');
 
-    int installationResult = -1;
-    Uri localObservatoryUri;
-
-    final Status installStatus = logger.startProgress('Installing and launching...', timeout: timeoutConfiguration.slowOperation);
-
-    if (!debuggingOptions.debuggingEnabled) {
-      // If debugging is not enabled, just launch the application and continue.
-      printTrace('Debugging is not enabled');
-      installationResult = await const IOSDeploy().runApp(
-        deviceId: id,
-        bundlePath: bundle.path,
-        launchArguments: launchArguments,
-      );
-    } else {
-      // Debugging is enabled, look for the observatory server port post launch.
-      printTrace('Debugging is enabled, connecting to observatory');
-
-      // 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.
-      final ProtocolDiscovery observatoryDiscovery = ProtocolDiscovery.observatory(
-        getLogReader(app: package),
-        portForwarder: portForwarder,
-        hostPort: debuggingOptions.observatoryPort,
-        ipv6: ipv6,
-      );
-
-      final Future<Uri> forwardObservatoryUri = observatoryDiscovery.uri;
+    final Status installStatus = logger.startProgress(
+        'Installing and launching...',
+        timeout: timeoutConfiguration.slowOperation);
+    try {
+      ProtocolDiscovery observatoryDiscovery;
+      if (debuggingOptions.debuggingEnabled) {
+        // Debugging is enabled, look for the observatory server port post launch.
+        printTrace('Debugging is enabled, connecting to observatory');
+
+        // 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.
+        observatoryDiscovery = ProtocolDiscovery.observatory(
+          getLogReader(app: package),
+          portForwarder: portForwarder,
+          hostPort: debuggingOptions.observatoryPort,
+          ipv6: ipv6,
+        );
+      }
 
-      final Future<int> launch = const IOSDeploy().runApp(
+      final int installationResult = await const IOSDeploy().runApp(
         deviceId: id,
         bundlePath: bundle.path,
         launchArguments: launchArguments,
       );
+      if (installationResult != 0) {
+        printError('Could not install ${bundle.path} on $id.');
+        printError('Try launching Xcode and selecting "Product > Run" to fix the problem:');
+        printError('  open ios/Runner.xcworkspace');
+        printError('');
+        return LaunchResult.failed();
+      }
 
-      localObservatoryUri = await launch.then<Uri>((int result) async {
-        installationResult = result;
-
-        if (result != 0) {
-          printTrace('Failed to launch the application on device.');
-          return null;
-        }
+      if (!debuggingOptions.debuggingEnabled) {
+        return LaunchResult.succeeded();
+      }
 
+      try {
         printTrace('Application launched on the device. Waiting for observatory port.');
-        return await forwardObservatoryUri;
-      }).whenComplete(() {
-        observatoryDiscovery.cancel();
-      });
-    }
-    installStatus.stop();
-
-    if (installationResult != 0) {
-      printError('Could not install ${bundle.path} on $id.');
-      printError('Try launching Xcode and selecting "Product > Run" to fix the problem:');
-      printError('  open ios/Runner.xcworkspace');
-      printError('');
-      return LaunchResult.failed();
+        final Uri localUri = await observatoryDiscovery.uri;
+        return LaunchResult.succeeded(observatoryUri: localUri);
+      } catch (error) {
+        printError('Failed to establish a debug connection with $id: $error');
+        return LaunchResult.failed();
+      } finally {
+        await observatoryDiscovery?.cancel();
+      }
+    } finally {
+      installStatus.stop();
     }
-
-    return LaunchResult.succeeded(observatoryUri: localObservatoryUri);
   }
 
   @override
@@ -417,13 +407,24 @@ class IOSDevice extends Device {
 
   @override
   DeviceLogReader getLogReader({ ApplicationPackage app }) {
-    _logReaders ??= <ApplicationPackage, _IOSDeviceLogReader>{};
+    _logReaders ??= <ApplicationPackage, DeviceLogReader>{};
     return _logReaders.putIfAbsent(app, () => _IOSDeviceLogReader(this, app));
   }
 
+  @visibleForTesting
+  void setLogReader(ApplicationPackage app, DeviceLogReader logReader) {
+    _logReaders ??= <ApplicationPackage, DeviceLogReader>{};
+    _logReaders[app] = logReader;
+  }
+
   @override
   DevicePortForwarder get portForwarder => _portForwarder ??= _IOSDevicePortForwarder(this);
 
+  @visibleForTesting
+  set portForwarder(DevicePortForwarder forwarder) {
+    _portForwarder = forwarder;
+  }
+
   @override
   void clearLogs() { }
 

packages/flutter_tools/lib/src/protocol_discovery.dart

@@ -65,9 +65,9 @@ class ProtocolDiscovery {
     if (match != null) {
       try {
         uri = Uri.parse(match[1]);
-      } catch (error) {
+      } catch (error, stackTrace) {
         _stopScrapingLogs();
-        _completer.completeError(error);
+        _completer.completeError(error, stackTrace);
       }
     }
 

packages/flutter_tools/test/general.shard/commands/attach_test.dart

@@ -52,15 +52,6 @@ void main() {
         mockLogReader = MockDeviceLogReader();
         portForwarder = MockPortForwarder();
         device = MockAndroidDevice();
-        when(device.getLogReader()).thenAnswer((_) {
-          // Now that the reader is used, start writing messages to it.
-          Timer.run(() {
-            mockLogReader.addLine('Foo');
-            mockLogReader.addLine('Observatory listening on http://127.0.0.1:$devicePort');
-          });
-
-          return mockLogReader;
-        });
         when(device.portForwarder)
           .thenReturn(portForwarder);
         when(portForwarder.forward(devicePort, hostPort: anyNamed('hostPort')))
@@ -82,6 +73,14 @@ void main() {
       });
 
       testUsingContext('finds observatory port and forwards', () async {
+        when(device.getLogReader()).thenAnswer((_) {
+          // Now that the reader is used, start writing messages to it.
+          Timer.run(() {
+            mockLogReader.addLine('Foo');
+            mockLogReader.addLine('Observatory listening on http://127.0.0.1:$devicePort');
+          });
+          return mockLogReader;
+        });
         testDeviceManager.addDevice(device);
         final Completer<void> completer = Completer<void>();
         final StreamSubscription<String> loggerSubscription = logger.stream.listen((String message) {
@@ -102,7 +101,32 @@ void main() {
         Logger: () => logger,
       });
 
+      testUsingContext('Fails with tool exit on bad Observatory uri', () async {
+        when(device.getLogReader()).thenAnswer((_) {
+          // Now that the reader is used, start writing messages to it.
+          Timer.run(() {
+            mockLogReader.addLine('Foo');
+            mockLogReader.addLine('Observatory listening on http:/:/127.0.0.1:$devicePort');
+          });
+          return mockLogReader;
+        });
+        testDeviceManager.addDevice(device);
+        expect(createTestCommandRunner(AttachCommand()).run(<String>['attach']),
+               throwsA(isA<ToolExit>()));
+      }, overrides: <Type, Generator>{
+        FileSystem: () => testFileSystem,
+        Logger: () => logger,
+      });
+
       testUsingContext('accepts filesystem parameters', () async {
+        when(device.getLogReader()).thenAnswer((_) {
+          // Now that the reader is used, start writing messages to it.
+          Timer.run(() {
+            mockLogReader.addLine('Foo');
+            mockLogReader.addLine('Observatory listening on http://127.0.0.1:$devicePort');
+          });
+          return mockLogReader;
+        });
         testDeviceManager.addDevice(device);
 
         const String filesystemScheme = 'foo';
@@ -175,6 +199,14 @@ void main() {
       });
 
       testUsingContext('exits when ipv6 is specified and debug-port is not', () async {
+        when(device.getLogReader()).thenAnswer((_) {
+          // Now that the reader is used, start writing messages to it.
+          Timer.run(() {
+            mockLogReader.addLine('Foo');
+            mockLogReader.addLine('Observatory listening on http://127.0.0.1:$devicePort');
+          });
+          return mockLogReader;
+        });
         testDeviceManager.addDevice(device);
 
         final AttachCommand command = AttachCommand();
@@ -190,6 +222,14 @@ void main() {
       },);
 
       testUsingContext('exits when observatory-port is specified and debug-port is not', () async {
+        when(device.getLogReader()).thenAnswer((_) {
+          // Now that the reader is used, start writing messages to it.
+          Timer.run(() {
+            mockLogReader.addLine('Foo');
+            mockLogReader.addLine('Observatory listening on http://127.0.0.1:$devicePort');
+          });
+          return mockLogReader;
+        });
         testDeviceManager.addDevice(device);
 
         final AttachCommand command = AttachCommand();

packages/flutter_tools/test/general.shard/ios/devices_test.dart

@@ -10,6 +10,7 @@ import 'package:flutter_tools/src/application_package.dart';
 import 'package:flutter_tools/src/artifacts.dart';
 import 'package:flutter_tools/src/base/file_system.dart';
 import 'package:flutter_tools/src/base/io.dart';
+import 'package:flutter_tools/src/build_info.dart';
 import 'package:flutter_tools/src/cache.dart';
 import 'package:flutter_tools/src/device.dart';
 import 'package:flutter_tools/src/ios/devices.dart';
@@ -30,10 +31,9 @@ class MockCache extends Mock implements Cache {}
 class MockDirectory extends Mock implements Directory {}
 class MockFileSystem extends Mock implements FileSystem {}
 class MockIMobileDevice extends Mock implements IMobileDevice {}
-class MockProcessManager extends Mock implements ProcessManager {}
 class MockXcode extends Mock implements Xcode {}
 class MockFile extends Mock implements File {}
-class MockProcess extends Mock implements Process {}
+class MockPortForwarder extends Mock implements DevicePortForwarder {}
 
 void main() {
   final FakePlatform macPlatform = FakePlatform.fromPlatform(const LocalPlatform());
@@ -63,6 +63,147 @@ void main() {
       });
     }
 
+    group('startApp', () {
+      MockIOSApp mockApp;
+      MockArtifacts mockArtifacts;
+      MockCache mockCache;
+      MockFileSystem mockFileSystem;
+      MockProcessManager mockProcessManager;
+      MockDeviceLogReader mockLogReader;
+      MockPortForwarder mockPortForwarder;
+
+      const int devicePort = 499;
+      const int hostPort = 42;
+      const String installerPath = '/path/to/ideviceinstaller';
+      const String iosDeployPath = '/path/to/iosdeploy';
+      // const String appId = '789';
+      const MapEntry<String, String> libraryEntry = MapEntry<String, String>(
+          'DYLD_LIBRARY_PATH',
+          '/path/to/libraries'
+      );
+      final Map<String, String> env = Map<String, String>.fromEntries(
+          <MapEntry<String, String>>[libraryEntry]
+      );
+
+      setUp(() {
+        mockApp = MockIOSApp();
+        mockArtifacts = MockArtifacts();
+        mockCache = MockCache();
+        when(mockCache.dyLdLibEntry).thenReturn(libraryEntry);
+        mockFileSystem = MockFileSystem();
+        mockProcessManager = MockProcessManager();
+        mockLogReader = MockDeviceLogReader();
+        mockPortForwarder = MockPortForwarder();
+
+        when(
+            mockArtifacts.getArtifactPath(
+                Artifact.ideviceinstaller,
+                platform: anyNamed('platform'),
+            )
+        ).thenReturn(installerPath);
+
+        when(
+            mockArtifacts.getArtifactPath(
+                Artifact.iosDeploy,
+                platform: anyNamed('platform'),
+            )
+        ).thenReturn(iosDeployPath);
+
+        when(mockPortForwarder.forward(devicePort, hostPort: anyNamed('hostPort')))
+          .thenAnswer((_) async => hostPort);
+        when(mockPortForwarder.forwardedPorts)
+          .thenReturn(<ForwardedPort>[ForwardedPort(hostPort, devicePort)]);
+        when(mockPortForwarder.unforward(any))
+          .thenAnswer((_) async => null);
+
+        const String bundlePath = '/path/to/bundle';
+        final List<String> installArgs = <String>[installerPath, '-i', bundlePath];
+        when(mockApp.deviceBundlePath).thenReturn(bundlePath);
+        final MockDirectory directory = MockDirectory();
+        when(mockFileSystem.directory(bundlePath)).thenReturn(directory);
+        when(directory.existsSync()).thenReturn(true);
+        when(mockProcessManager.run(installArgs, environment: env))
+            .thenAnswer(
+                (_) => Future<ProcessResult>.value(ProcessResult(1, 0, '', ''))
+            );
+      });
+
+      tearDown(() {
+        mockLogReader.dispose();
+      });
+
+      testUsingContext(' succeeds in debug mode', () async {
+        final IOSDevice device = IOSDevice('123');
+        device.portForwarder = mockPortForwarder;
+        device.setLogReader(mockApp, mockLogReader);
+
+        // Now that the reader is used, start writing messages to it.
+        Timer.run(() {
+          mockLogReader.addLine('Foo');
+          mockLogReader.addLine('Observatory listening on http://127.0.0.1:$devicePort');
+        });
+
+        final LaunchResult launchResult = await device.startApp(mockApp,
+          prebuiltApplication: true,
+          debuggingOptions: DebuggingOptions.enabled(const BuildInfo(BuildMode.debug, null)),
+          platformArgs: <String, dynamic>{},
+        );
+        expect(launchResult.started, isTrue);
+        expect(launchResult.hasObservatory, isTrue);
+        expect(await device.stopApp(mockApp), isFalse);
+      }, overrides: <Type, Generator>{
+        Artifacts: () => mockArtifacts,
+        Cache: () => mockCache,
+        FileSystem: () => mockFileSystem,
+        Platform: () => macPlatform,
+        ProcessManager: () => mockProcessManager,
+      });
+
+      testUsingContext(' succeeds in release mode', () async {
+        final IOSDevice device = IOSDevice('123');
+        final LaunchResult launchResult = await device.startApp(mockApp,
+          prebuiltApplication: true,
+          debuggingOptions: DebuggingOptions.disabled(const BuildInfo(BuildMode.release, null)),
+          platformArgs: <String, dynamic>{},
+        );
+        expect(launchResult.started, isTrue);
+        expect(launchResult.hasObservatory, isFalse);
+        expect(await device.stopApp(mockApp), isFalse);
+      }, overrides: <Type, Generator>{
+        Artifacts: () => mockArtifacts,
+        Cache: () => mockCache,
+        FileSystem: () => mockFileSystem,
+        Platform: () => macPlatform,
+        ProcessManager: () => mockProcessManager,
+      });
+
+      testUsingContext(' fails in debug mode when Observatory URI is malformed', () async {
+        final IOSDevice device = IOSDevice('123');
+        device.portForwarder = mockPortForwarder;
+        device.setLogReader(mockApp, mockLogReader);
+
+        // Now that the reader is used, start writing messages to it.
+        Timer.run(() {
+          mockLogReader.addLine('Foo');
+          mockLogReader.addLine('Observatory listening on http:/:/127.0.0.1:$devicePort');
+        });
+
+        final LaunchResult launchResult = await device.startApp(mockApp,
+            prebuiltApplication: true,
+            debuggingOptions: DebuggingOptions.enabled(const BuildInfo(BuildMode.debug, null)),
+            platformArgs: <String, dynamic>{},
+        );
+        expect(launchResult.started, isFalse);
+        expect(launchResult.hasObservatory, isFalse);
+      }, overrides: <Type, Generator>{
+        Artifacts: () => mockArtifacts,
+        Cache: () => mockCache,
+        FileSystem: () => mockFileSystem,
+        Platform: () => macPlatform,
+        ProcessManager: () => mockProcessManager,
+      });
+    });
+
     group('Process calls', () {
       MockIOSApp mockApp;
       MockArtifacts mockArtifacts;
@@ -263,20 +404,15 @@ f577a7903cc54959be2e34bc4f7f80b7009efcf4
 
     testUsingContext('suppresses non-Flutter lines from output', () async {
       when(mockIMobileDevice.startLogger('123456')).thenAnswer((Invocation invocation) {
-        final Process mockProcess = MockProcess();
-        when(mockProcess.stdout).thenAnswer((Invocation invocation) =>
-            Stream<List<int>>.fromIterable(<List<int>>['''
-  Runner(Flutter)[297] <Notice>: A is for ari
-  Runner(libsystem_asl.dylib)[297] <Notice>: libMobileGestalt MobileGestaltSupport.m:153: pid 123 (Runner) does not have sandbox access for frZQaeyWLUvLjeuEK43hmg and IS NOT appropriately entitled
-  Runner(libsystem_asl.dylib)[297] <Notice>: libMobileGestalt MobileGestalt.c:550: no access to InverseDeviceID (see <rdar://problem/11744455>)
-  Runner(Flutter)[297] <Notice>: I is for ichigo
-  Runner(UIKit)[297] <Notice>: E is for enpitsu"
-  '''.codeUnits]));
-        when(mockProcess.stderr)
-            .thenAnswer((Invocation invocation) => const Stream<List<int>>.empty());
-        // Delay return of exitCode until after stdout stream data, since it terminates the logger.
-        when(mockProcess.exitCode)
-            .thenAnswer((Invocation invocation) => Future<int>.delayed(Duration.zero, () => 0));
+        final Process mockProcess = MockProcess(
+          stdout: Stream<List<int>>.fromIterable(<List<int>>['''
+Runner(Flutter)[297] <Notice>: A is for ari
+Runner(libsystem_asl.dylib)[297] <Notice>: libMobileGestalt MobileGestaltSupport.m:153: pid 123 (Runner) does not have sandbox access for frZQaeyWLUvLjeuEK43hmg and IS NOT appropriately entitled
+Runner(libsystem_asl.dylib)[297] <Notice>: libMobileGestalt MobileGestalt.c:550: no access to InverseDeviceID (see <rdar://problem/11744455>)
+Runner(Flutter)[297] <Notice>: I is for ichigo
+Runner(UIKit)[297] <Notice>: E is for enpitsu"
+'''.codeUnits])
+        );
         return Future<Process>.value(mockProcess);
       });
 
@@ -293,20 +429,15 @@ f577a7903cc54959be2e34bc4f7f80b7009efcf4
     });
     testUsingContext('includes multi-line Flutter logs in the output', () async {
       when(mockIMobileDevice.startLogger('123456')).thenAnswer((Invocation invocation) {
-        final Process mockProcess = MockProcess();
-        when(mockProcess.stdout).thenAnswer((Invocation invocation) =>
-            Stream<List<int>>.fromIterable(<List<int>>['''
-  Runner(Flutter)[297] <Notice>: This is a multi-line message,
+        final Process mockProcess = MockProcess(
+          stdout: Stream<List<int>>.fromIterable(<List<int>>['''
+Runner(Flutter)[297] <Notice>: This is a multi-line message,
   with another Flutter message following it.
-  Runner(Flutter)[297] <Notice>: This is a multi-line message,
+Runner(Flutter)[297] <Notice>: This is a multi-line message,
   with a non-Flutter log message following it.
-  Runner(libsystem_asl.dylib)[297] <Notice>: libMobileGestalt
-  '''.codeUnits]));
-        when(mockProcess.stderr)
-            .thenAnswer((Invocation invocation) => const Stream<List<int>>.empty());
-        // Delay return of exitCode until after stdout stream data, since it terminates the logger.
-        when(mockProcess.exitCode)
-            .thenAnswer((Invocation invocation) => Future<int>.delayed(Duration.zero, () => 0));
+Runner(libsystem_asl.dylib)[297] <Notice>: libMobileGestalt
+'''.codeUnits]),
+        );
         return Future<Process>.value(mockProcess);
       });
 

packages/flutter_tools/test/src/mocks.dart

@@ -153,7 +153,7 @@ ro.build.version.codename=REL
 typedef ProcessFactory = Process Function(List<String> command);
 
 /// A ProcessManager that starts Processes by delegating to a ProcessFactory.
-class MockProcessManager implements ProcessManager {
+class MockProcessManager extends Mock implements ProcessManager {
   ProcessFactory processFactory = (List<String> commands) => MockProcess();
   bool canRunSucceeds = true;
   bool runSucceeds = true;
@@ -180,9 +180,6 @@ class MockProcessManager implements ProcessManager {
     commands = command;
     return Future<Process>.value(processFactory(command));
   }
-
-  @override
-  dynamic noSuchMethod(Invocation invocation) => null;
 }
 
 /// A process that exits successfully with no output and ignores all input.