[flutter_tool] Observatory connection error handling cleanup (#38353)
diff --git a/packages/flutter_tools/lib/src/commands/attach.dart b/packages/flutter_tools/lib/src/commands/attach.dart index a9b1caf..dd68a47 100644 --- a/packages/flutter_tools/lib/src/commands/attach.dart +++ b/packages/flutter_tools/lib/src/commands/attach.dart
@@ -243,6 +243,8 @@ // 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(); }
diff --git a/packages/flutter_tools/lib/src/ios/devices.dart b/packages/flutter_tools/lib/src/ios/devices.dart index a6594d7..0d7714a 100644 --- a/packages/flutter_tools/lib/src/ios/devices.dart +++ b/packages/flutter_tools/lib/src/ios/devices.dart
@@ -152,9 +152,9 @@ @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 @@ 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); + try { + ProtocolDiscovery observatoryDiscovery; + if (debuggingOptions.debuggingEnabled) { + // Debugging is enabled, look for the observatory server port post launch. + printTrace('Debugging is enabled, connecting to observatory'); - final Status installStatus = logger.startProgress('Installing and launching...', timeout: timeoutConfiguration.slowOperation); + // 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, + ); + } - if (!debuggingOptions.debuggingEnabled) { - // If debugging is not enabled, just launch the application and continue. - printTrace('Debugging is not enabled'); - installationResult = await const IOSDeploy().runApp( + final int 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'); + 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(); + } - // 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, - ); + if (!debuggingOptions.debuggingEnabled) { + return LaunchResult.succeeded(); + } - final Future<Uri> forwardObservatoryUri = observatoryDiscovery.uri; - - final Future<int> launch = const IOSDeploy().runApp( - deviceId: id, - bundlePath: bundle.path, - launchArguments: launchArguments, - ); - - localObservatoryUri = await launch.then<Uri>((int result) async { - installationResult = result; - - if (result != 0) { - printTrace('Failed to launch the application on device.'); - return null; - } - + try { printTrace('Application launched on the device. Waiting for observatory port.'); - return await forwardObservatoryUri; - }).whenComplete(() { - observatoryDiscovery.cancel(); - }); + 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(); } - 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(); - } - - return LaunchResult.succeeded(observatoryUri: localObservatoryUri); } @override @@ -417,13 +407,24 @@ @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() { }
diff --git a/packages/flutter_tools/lib/src/protocol_discovery.dart b/packages/flutter_tools/lib/src/protocol_discovery.dart index 96ea8d2..81789ee 100644 --- a/packages/flutter_tools/lib/src/protocol_discovery.dart +++ b/packages/flutter_tools/lib/src/protocol_discovery.dart
@@ -65,9 +65,9 @@ if (match != null) { try { uri = Uri.parse(match[1]); - } catch (error) { + } catch (error, stackTrace) { _stopScrapingLogs(); - _completer.completeError(error); + _completer.completeError(error, stackTrace); } }
diff --git a/packages/flutter_tools/test/general.shard/commands/attach_test.dart b/packages/flutter_tools/test/general.shard/commands/attach_test.dart index bfe29a9..23a1a47 100644 --- a/packages/flutter_tools/test/general.shard/commands/attach_test.dart +++ b/packages/flutter_tools/test/general.shard/commands/attach_test.dart
@@ -52,15 +52,6 @@ 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 @@ }); 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 @@ 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 @@ }); 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 @@ },); 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();
diff --git a/packages/flutter_tools/test/general.shard/ios/devices_test.dart b/packages/flutter_tools/test/general.shard/ios/devices_test.dart index 89075c6..b1d1b4b 100644 --- a/packages/flutter_tools/test/general.shard/ios/devices_test.dart +++ b/packages/flutter_tools/test/general.shard/ios/devices_test.dart
@@ -10,6 +10,7 @@ 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 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 @@ }); } + 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 @@ 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 @@ }); 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); });
diff --git a/packages/flutter_tools/test/src/mocks.dart b/packages/flutter_tools/test/src/mocks.dart index 3400110..92d184d 100644 --- a/packages/flutter_tools/test/src/mocks.dart +++ b/packages/flutter_tools/test/src/mocks.dart
@@ -153,7 +153,7 @@ 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 @@ 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.