From a40ab895cf4a6d07516a79321daabdb53ffe7d39 Mon Sep 17 00:00:00 2001 From: Zachary Anderson Date: Thu, 15 Aug 2019 12:13:03 -0700 Subject: [PATCH] [flutter_tool] Observatory connection error handling cleanup (#38353) --- .../lib/src/commands/attach.dart | 2 + .../flutter_tools/lib/src/ios/devices.dart | 105 +++++----- .../lib/src/protocol_discovery.dart | 4 +- .../general.shard/commands/attach_test.dart | 58 +++++- .../test/general.shard/ios/devices_test.dart | 187 +++++++++++++++--- packages/flutter_tools/test/src/mocks.dart | 5 +- 6 files changed, 266 insertions(+), 95 deletions(-) diff --git a/packages/flutter_tools/lib/src/commands/attach.dart b/packages/flutter_tools/lib/src/commands/attach.dart index a9b1caf5c7..dd68a476a8 100644 --- a/packages/flutter_tools/lib/src/commands/attach.dart +++ b/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(); } diff --git a/packages/flutter_tools/lib/src/ios/devices.dart b/packages/flutter_tools/lib/src/ios/devices.dart index a6594d7055..0d7714a4fb 100644 --- a/packages/flutter_tools/lib/src/ios/devices.dart +++ b/packages/flutter_tools/lib/src/ios/devices.dart @@ -152,9 +152,9 @@ class IOSDevice extends Device { @override final String name; - Map _logReaders; + Map _logReaders; - _IOSDevicePortForwarder _portForwarder; + DevicePortForwarder _portForwarder; @override Future 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); + 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, - ); - - final Future forwardObservatoryUri = observatoryDiscovery.uri; - - final Future launch = const IOSDeploy().runApp( - deviceId: id, - bundlePath: bundle.path, - launchArguments: launchArguments, - ); - - localObservatoryUri = await launch.then((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(); - }); + 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 @@ class IOSDevice extends Device { @override DeviceLogReader getLogReader({ ApplicationPackage app }) { - _logReaders ??= {}; + _logReaders ??= {}; return _logReaders.putIfAbsent(app, () => _IOSDeviceLogReader(this, app)); } + @visibleForTesting + void setLogReader(ApplicationPackage app, DeviceLogReader logReader) { + _logReaders ??= {}; + _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 96ea8d2bba..81789eebda 100644 --- a/packages/flutter_tools/lib/src/protocol_discovery.dart +++ b/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); } } 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 bfe29a98fe..23a1a478b1 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 @@ 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 completer = Completer(); final StreamSubscription 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(['attach']), + throwsA(isA())); + }, overrides: { + 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(); 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 89075c65b2..b1d1b4b2fb 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/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 libraryEntry = MapEntry( + 'DYLD_LIBRARY_PATH', + '/path/to/libraries' + ); + final Map env = Map.fromEntries( + >[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(hostPort, devicePort)]); + when(mockPortForwarder.unforward(any)) + .thenAnswer((_) async => null); + + const String bundlePath = '/path/to/bundle'; + final List installArgs = [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.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: {}, + ); + expect(launchResult.started, isTrue); + expect(launchResult.hasObservatory, isTrue); + expect(await device.stopApp(mockApp), isFalse); + }, overrides: { + 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: {}, + ); + expect(launchResult.started, isTrue); + expect(launchResult.hasObservatory, isFalse); + expect(await device.stopApp(mockApp), isFalse); + }, overrides: { + 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: {}, + ); + expect(launchResult.started, isFalse); + expect(launchResult.hasObservatory, isFalse); + }, overrides: { + 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>.fromIterable(>[''' - Runner(Flutter)[297] : A is for ari - Runner(libsystem_asl.dylib)[297] : libMobileGestalt MobileGestaltSupport.m:153: pid 123 (Runner) does not have sandbox access for frZQaeyWLUvLjeuEK43hmg and IS NOT appropriately entitled - Runner(libsystem_asl.dylib)[297] : libMobileGestalt MobileGestalt.c:550: no access to InverseDeviceID (see ) - Runner(Flutter)[297] : I is for ichigo - Runner(UIKit)[297] : E is for enpitsu" - '''.codeUnits])); - when(mockProcess.stderr) - .thenAnswer((Invocation invocation) => const Stream>.empty()); - // Delay return of exitCode until after stdout stream data, since it terminates the logger. - when(mockProcess.exitCode) - .thenAnswer((Invocation invocation) => Future.delayed(Duration.zero, () => 0)); + final Process mockProcess = MockProcess( + stdout: Stream>.fromIterable(>[''' +Runner(Flutter)[297] : A is for ari +Runner(libsystem_asl.dylib)[297] : libMobileGestalt MobileGestaltSupport.m:153: pid 123 (Runner) does not have sandbox access for frZQaeyWLUvLjeuEK43hmg and IS NOT appropriately entitled +Runner(libsystem_asl.dylib)[297] : libMobileGestalt MobileGestalt.c:550: no access to InverseDeviceID (see ) +Runner(Flutter)[297] : I is for ichigo +Runner(UIKit)[297] : E is for enpitsu" +'''.codeUnits]) + ); return Future.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>.fromIterable(>[''' - Runner(Flutter)[297] : This is a multi-line message, + final Process mockProcess = MockProcess( + stdout: Stream>.fromIterable(>[''' +Runner(Flutter)[297] : This is a multi-line message, with another Flutter message following it. - Runner(Flutter)[297] : This is a multi-line message, +Runner(Flutter)[297] : This is a multi-line message, with a non-Flutter log message following it. - Runner(libsystem_asl.dylib)[297] : libMobileGestalt - '''.codeUnits])); - when(mockProcess.stderr) - .thenAnswer((Invocation invocation) => const Stream>.empty()); - // Delay return of exitCode until after stdout stream data, since it terminates the logger. - when(mockProcess.exitCode) - .thenAnswer((Invocation invocation) => Future.delayed(Duration.zero, () => 0)); +Runner(libsystem_asl.dylib)[297] : libMobileGestalt +'''.codeUnits]), + ); return Future.value(mockProcess); }); diff --git a/packages/flutter_tools/test/src/mocks.dart b/packages/flutter_tools/test/src/mocks.dart index 34001108b5..92d184d01e 100644 --- a/packages/flutter_tools/test/src/mocks.dart +++ b/packages/flutter_tools/test/src/mocks.dart @@ -153,7 +153,7 @@ ro.build.version.codename=REL typedef ProcessFactory = Process Function(List command); /// A ProcessManager that starts Processes by delegating to a ProcessFactory. -class MockProcessManager implements ProcessManager { +class MockProcessManager extends Mock implements ProcessManager { ProcessFactory processFactory = (List commands) => MockProcess(); bool canRunSucceeds = true; bool runSucceeds = true; @@ -180,9 +180,6 @@ class MockProcessManager implements ProcessManager { commands = command; return Future.value(processFactory(command)); } - - @override - dynamic noSuchMethod(Invocation invocation) => null; } /// A process that exits successfully with no output and ignores all input.