From 89ef88c64f4d4b4fe81d44174de0e0abb442817f Mon Sep 17 00:00:00 2001 From: Ben Konyi Date: Fri, 4 Dec 2020 17:16:30 -0800 Subject: [PATCH] Ensure attaching to an application with an existing DDS instance is not treated as a fatal error (#70847) --- .../gradle_deprecated_settings/pubspec.yaml | 4 +- dev/tools/pubspec.yaml | 4 +- packages/flutter_tools/lib/src/base/dds.dart | 8 +++- .../lib/src/commands/attach.dart | 10 ++++- .../lib/src/isolated/resident_web_runner.dart | 1 + .../lib/src/resident_runner.dart | 28 ++++++++++--- packages/flutter_tools/lib/src/run_cold.dart | 2 + packages/flutter_tools/lib/src/run_hot.dart | 2 + packages/flutter_tools/pubspec.yaml | 4 +- .../commands.shard/hermetic/attach_test.dart | 6 +-- .../test/general.shard/cold_test.dart | 1 + .../test/general.shard/hot_test.dart | 1 + .../general.shard/resident_runner_test.dart | 40 +++++++++++++++++++ .../general.shard/terminal_handler_test.dart | 1 + 14 files changed, 96 insertions(+), 16 deletions(-) diff --git a/dev/integration_tests/gradle_deprecated_settings/pubspec.yaml b/dev/integration_tests/gradle_deprecated_settings/pubspec.yaml index 793897e308..efb9fdcc1c 100644 --- a/dev/integration_tests/gradle_deprecated_settings/pubspec.yaml +++ b/dev/integration_tests/gradle_deprecated_settings/pubspec.yaml @@ -8,7 +8,7 @@ environment: dependencies: flutter: sdk: flutter - camera: 0.5.8+11 + camera: 0.5.8+17 characters: 1.1.0-nullsafety.5 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade" collection: 1.15.0-nullsafety.5 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade" @@ -19,4 +19,4 @@ dependencies: flutter: uses-material-design: true -# PUBSPEC CHECKSUM: aa03 +# PUBSPEC CHECKSUM: 8f09 diff --git a/dev/tools/pubspec.yaml b/dev/tools/pubspec.yaml index 9cdc948e41..995e11aa30 100644 --- a/dev/tools/pubspec.yaml +++ b/dev/tools/pubspec.yaml @@ -20,7 +20,7 @@ dependencies: analyzer: 0.40.6 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade" async: 2.5.0-nullsafety.3 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade" boolean_selector: 2.1.0-nullsafety.3 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade" - browser_launcher: 0.1.7 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade" + browser_launcher: 0.1.8 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade" built_collection: 4.3.2 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade" built_value: 7.1.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade" charcode: 1.2.0-nullsafety.3 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade" @@ -97,4 +97,4 @@ dev_dependencies: node_preamble: 1.4.12 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade" -# PUBSPEC CHECKSUM: bb6f +# PUBSPEC CHECKSUM: 7770 diff --git a/packages/flutter_tools/lib/src/base/dds.dart b/packages/flutter_tools/lib/src/base/dds.dart index 7d052eec41..a7d6af4655 100644 --- a/packages/flutter_tools/lib/src/base/dds.dart +++ b/packages/flutter_tools/lib/src/base/dds.dart @@ -19,7 +19,8 @@ class DartDevelopmentService { final Logger logger; dds.DartDevelopmentService _ddsInstance; - Uri get uri => _ddsInstance.uri; + Uri get uri => _ddsInstance?.uri ?? _existingDdsUri; + Uri _existingDdsUri; Future get done => _completer.future; final Completer _completer = Completer(); @@ -57,6 +58,11 @@ class DartDevelopmentService { logger.printTrace('DDS is listening at ${_ddsInstance.uri}.'); } on dds.DartDevelopmentServiceException catch (e) { logger.printTrace('Warning: Failed to start DDS: ${e.message}'); + if (e.errorCode == dds.DartDevelopmentServiceException.existingDdsInstanceError) { + _existingDdsUri = Uri.parse( + e.message.split(' ').firstWhere((String e) => e.startsWith('http')) + ); + } if (!_completer.isCompleted) { _completer.complete(); } diff --git a/packages/flutter_tools/lib/src/commands/attach.dart b/packages/flutter_tools/lib/src/commands/attach.dart index d7dc29d841..d10a7000e2 100644 --- a/packages/flutter_tools/lib/src/commands/attach.dart +++ b/packages/flutter_tools/lib/src/commands/attach.dart @@ -318,7 +318,14 @@ known, it can be explicitly provided to attach via the command-line, e.g. try { app = await daemon.appDomain.launch( runner, - runner.attach, + ({Completer connectionInfoCompleter, + Completer appStartedCompleter}) { + return runner.attach( + connectionInfoCompleter: connectionInfoCompleter, + appStartedCompleter: appStartedCompleter, + allowExistingDdsInstance: true, + ); + }, device, null, true, @@ -354,6 +361,7 @@ known, it can be explicitly provided to attach via the command-line, e.g. })); result = await runner.attach( appStartedCompleter: onAppStart, + allowExistingDdsInstance: true, ); if (result != 0) { throwToolExit(null, exitCode: result); diff --git a/packages/flutter_tools/lib/src/isolated/resident_web_runner.dart b/packages/flutter_tools/lib/src/isolated/resident_web_runner.dart index 7a016ffa79..c320c3e7cf 100644 --- a/packages/flutter_tools/lib/src/isolated/resident_web_runner.dart +++ b/packages/flutter_tools/lib/src/isolated/resident_web_runner.dart @@ -748,6 +748,7 @@ class _ResidentWebRunner extends ResidentWebRunner { Future attach({ Completer connectionInfoCompleter, Completer appStartedCompleter, + bool allowExistingDdsInstance = false, }) async { if (_chromiumLauncher != null) { final Chromium chrome = await _chromiumLauncher.connectedInstance; diff --git a/packages/flutter_tools/lib/src/resident_runner.dart b/packages/flutter_tools/lib/src/resident_runner.dart index 633c63b346..55f74faf82 100644 --- a/packages/flutter_tools/lib/src/resident_runner.dart +++ b/packages/flutter_tools/lib/src/resident_runner.dart @@ -4,6 +4,7 @@ import 'dart:async'; +import 'package:dds/dds.dart' as dds; import 'package:meta/meta.dart'; import 'package:package_config/package_config.dart'; import 'package:vm_service/vm_service.dart' as vm_service; @@ -221,6 +222,7 @@ class FlutterDevice { int ddsPort, bool disableServiceAuthCodes = false, bool disableDds = false, + bool allowExistingDdsInstance = false, bool ipv6 = false, }) { final Completer completer = Completer(); @@ -231,8 +233,15 @@ class FlutterDevice { // FYI, this message is used as a sentinel in tests. globals.printTrace('Connecting to service protocol: $observatoryUri'); isWaitingForVm = true; + bool existingDds = false; vm_service.VmService service; if (!disableDds) { + void handleError(Exception e) { + globals.printTrace('Fail to connect to service protocol: $observatoryUri: $e'); + if (!completer.isCompleted) { + completer.completeError('failed to connect to $observatoryUri'); + } + } // This first try block is meant to catch errors that occur during DDS startup // (e.g., failure to bind to a port, failure to connect to the VM service, // attaching to a VM service with existing clients, etc.). @@ -243,11 +252,16 @@ class FlutterDevice { ipv6, disableServiceAuthCodes, ); - } on Exception catch (e) { - globals.printTrace('Fail to connect to service protocol: $observatoryUri: $e'); - if (!completer.isCompleted && !_isListeningForObservatoryUri) { - completer.completeError('failed to connect to $observatoryUri'); + } on dds.DartDevelopmentServiceException catch (e) { + if (!allowExistingDdsInstance || + (e.errorCode != dds.DartDevelopmentServiceException.existingDdsInstanceError)) { + handleError(e); + return; + } else { + existingDds = true; } + } on Exception catch (e) { + handleError(e); return; } } @@ -267,7 +281,8 @@ class FlutterDevice { printStructuredErrorLogMethod: printStructuredErrorLogMethod, device: device, ), - device.dds.done.whenComplete(() => throw Exception('DDS shut down too early')), + if (!existingDds) + device.dds.done.whenComplete(() => throw Exception('DDS shut down too early')), ] ) as vm_service.VmService; } on Exception catch (exception) { @@ -849,6 +864,7 @@ abstract class ResidentRunner { Future attach({ Completer connectionInfoCompleter, Completer appStartedCompleter, + bool allowExistingDdsInstance = false, }); bool get supportsRestart => false; @@ -1201,6 +1217,7 @@ abstract class ResidentRunner { Restart restart, CompileExpression compileExpression, GetSkSLMethod getSkSLMethod, + bool allowExistingDdsInstance, }) async { if (!debuggingOptions.debuggingEnabled) { throw 'The service protocol is not enabled.'; @@ -1214,6 +1231,7 @@ abstract class ResidentRunner { compileExpression: compileExpression, disableDds: debuggingOptions.disableDds, ddsPort: debuggingOptions.ddsPort, + allowExistingDdsInstance: allowExistingDdsInstance, hostVmServicePort: debuggingOptions.hostVmServicePort, getSkSLMethod: getSkSLMethod, printStructuredErrorLogMethod: printStructuredErrorLog, diff --git a/packages/flutter_tools/lib/src/run_cold.dart b/packages/flutter_tools/lib/src/run_cold.dart index 7576240a41..6ce44a13c5 100644 --- a/packages/flutter_tools/lib/src/run_cold.dart +++ b/packages/flutter_tools/lib/src/run_cold.dart @@ -128,11 +128,13 @@ class ColdRunner extends ResidentRunner { Future attach({ Completer connectionInfoCompleter, Completer appStartedCompleter, + bool allowExistingDdsInstance = false, }) async { _didAttach = true; try { await connectToServiceProtocol( getSkSLMethod: writeSkSL, + allowExistingDdsInstance: allowExistingDdsInstance, ); } on Exception catch (error) { globals.printError('Error connecting to the service protocol: $error'); diff --git a/packages/flutter_tools/lib/src/run_hot.dart b/packages/flutter_tools/lib/src/run_hot.dart index f81e1d60f3..fbb6cffa52 100644 --- a/packages/flutter_tools/lib/src/run_hot.dart +++ b/packages/flutter_tools/lib/src/run_hot.dart @@ -171,6 +171,7 @@ class HotRunner extends ResidentRunner { Future attach({ Completer connectionInfoCompleter, Completer appStartedCompleter, + bool allowExistingDdsInstance = false, }) async { _didAttach = true; try { @@ -179,6 +180,7 @@ class HotRunner extends ResidentRunner { restart: _restartService, compileExpression: _compileExpressionService, getSkSLMethod: writeSkSL, + allowExistingDdsInstance: allowExistingDdsInstance, ); // Catches all exceptions, non-Exception objects are rethrown. } catch (error) { // ignore: avoid_catches_without_on_clauses diff --git a/packages/flutter_tools/pubspec.yaml b/packages/flutter_tools/pubspec.yaml index 31c3939625..ab33da83da 100644 --- a/packages/flutter_tools/pubspec.yaml +++ b/packages/flutter_tools/pubspec.yaml @@ -62,7 +62,7 @@ dependencies: _fe_analyzer_shared: 12.0.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade" analyzer: 0.40.6 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade" boolean_selector: 2.1.0-nullsafety.3 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade" - browser_launcher: 0.1.7 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade" + browser_launcher: 0.1.8 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade" built_collection: 4.3.2 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade" built_value: 7.1.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade" charcode: 1.2.0-nullsafety.3 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade" @@ -111,4 +111,4 @@ dartdoc: # Exclude this package from the hosted API docs. nodoc: true -# PUBSPEC CHECKSUM: 2f49 +# PUBSPEC CHECKSUM: 8b4a diff --git a/packages/flutter_tools/test/commands.shard/hermetic/attach_test.dart b/packages/flutter_tools/test/commands.shard/hermetic/attach_test.dart index 725cf8b3a4..1b69f13964 100644 --- a/packages/flutter_tools/test/commands.shard/hermetic/attach_test.dart +++ b/packages/flutter_tools/test/commands.shard/hermetic/attach_test.dart @@ -179,7 +179,7 @@ void main() { const String outputDill = '/tmp/output.dill'; final MockHotRunner mockHotRunner = MockHotRunner(); - when(mockHotRunner.attach(appStartedCompleter: anyNamed('appStartedCompleter'))) + when(mockHotRunner.attach(appStartedCompleter: anyNamed('appStartedCompleter'), allowExistingDdsInstance: true)) .thenAnswer((_) async => 0); when(mockHotRunner.exited).thenReturn(false); when(mockHotRunner.isWaitingForObservatory).thenReturn(false); @@ -311,7 +311,7 @@ void main() { .thenReturn([ForwardedPort(hostPort, devicePort)]); when(portForwarder.unforward(any)) .thenAnswer((_) async {}); - when(mockHotRunner.attach(appStartedCompleter: anyNamed('appStartedCompleter'))) + when(mockHotRunner.attach(appStartedCompleter: anyNamed('appStartedCompleter'), allowExistingDdsInstance: true)) .thenAnswer((_) async => 0); when(mockHotRunnerFactory.build( any, @@ -395,7 +395,7 @@ void main() { .thenReturn([ForwardedPort(hostPort, devicePort)]); when(portForwarder.unforward(any)) .thenAnswer((_) async {}); - when(mockHotRunner.attach(appStartedCompleter: anyNamed('appStartedCompleter'))) + when(mockHotRunner.attach(appStartedCompleter: anyNamed('appStartedCompleter'), allowExistingDdsInstance: true)) .thenAnswer((_) async => 0); when(mockHotRunnerFactory.build( any, diff --git a/packages/flutter_tools/test/general.shard/cold_test.dart b/packages/flutter_tools/test/general.shard/cold_test.dart index 96afaccf3c..81a24948b9 100644 --- a/packages/flutter_tools/test/general.shard/cold_test.dart +++ b/packages/flutter_tools/test/general.shard/cold_test.dart @@ -129,6 +129,7 @@ class TestFlutterDevice extends FlutterDevice { int hostVmServicePort, int ddsPort, bool ipv6 = false, + bool allowExistingDdsInstance = false, }) async { throw exception; } diff --git a/packages/flutter_tools/test/general.shard/hot_test.dart b/packages/flutter_tools/test/general.shard/hot_test.dart index 4afe0ce09e..a18097f587 100644 --- a/packages/flutter_tools/test/general.shard/hot_test.dart +++ b/packages/flutter_tools/test/general.shard/hot_test.dart @@ -598,6 +598,7 @@ class TestFlutterDevice extends FlutterDevice { bool ipv6 = false, int hostVmServicePort, int ddsPort, + bool allowExistingDdsInstance = false, }) async { throw exception; } diff --git a/packages/flutter_tools/test/general.shard/resident_runner_test.dart b/packages/flutter_tools/test/general.shard/resident_runner_test.dart index a9fa3c129e..aaaab19411 100644 --- a/packages/flutter_tools/test/general.shard/resident_runner_test.dart +++ b/packages/flutter_tools/test/general.shard/resident_runner_test.dart @@ -10,6 +10,7 @@ import 'package:flutter_tools/src/features.dart'; import 'package:meta/meta.dart'; import 'package:package_config/package_config.dart'; import 'package:vm_service/vm_service.dart' as vm_service; +import 'package:dds/dds.dart' as dds; import 'package:file/memory.dart'; import 'package:file_testing/file_testing.dart'; import 'package:flutter_tools/src/artifacts.dart'; @@ -2673,6 +2674,36 @@ void main() { }) async => mockVMService, })); + testUsingContext('FlutterDevice handles existing DDS instance', () => testbed.run(() async { + fakeVmServiceHost = FakeVmServiceHost(requests: []); + final MockDevice mockDevice = MockDevice(); + final MockDartDevelopmentService mockDds = MockDartDevelopmentService(); + final MockDeviceLogReader mockLogReader = MockDeviceLogReader(); + final Completer noopCompleter = Completer(); + when(mockDevice.getLogReader(app: anyNamed('app'))).thenReturn(mockLogReader); + when(mockDevice.dds).thenReturn(mockDds); + when(mockDds.startDartDevelopmentService(any, any, any, any)).thenThrow(FakeDartDevelopmentServiceException()); + when(mockDds.done).thenAnswer((_) => noopCompleter.future); + + final TestFlutterDevice flutterDevice = TestFlutterDevice( + mockDevice, + observatoryUris: Stream.value(testUri), + ); + await flutterDevice.connect(allowExistingDdsInstance: true); + verify(mockLogReader.connectedVMService = mockVMService); + }, overrides: { + VMServiceConnector: () => (Uri httpUri, { + ReloadSources reloadSources, + Restart restart, + CompileExpression compileExpression, + GetSkSLMethod getSkSLMethod, + PrintStructuredErrorLogMethod printStructuredErrorLogMethod, + io.CompressionOptions compression, + Device device, + }) async => mockVMService, + })); + + testUsingContext('nextPlatform moves through expected platforms', () { expect(nextPlatform('android', TestFeatureFlags()), 'iOS'); expect(nextPlatform('iOS', TestFeatureFlags()), 'fuchsia'); @@ -2694,6 +2725,14 @@ class MockUsage extends Mock implements Usage {} class MockProcessManager extends Mock implements ProcessManager {} class MockResidentCompiler extends Mock implements ResidentCompiler {} +class FakeDartDevelopmentServiceException implements dds.DartDevelopmentServiceException { + @override + final int errorCode = dds.DartDevelopmentServiceException.existingDdsInstanceError; + + @override + final String message = 'A DDS instance is already connected at http://localhost:8181'; +} + class TestFlutterDevice extends FlutterDevice { TestFlutterDevice(Device device, { Stream observatoryUris }) : super(device, buildInfo: BuildInfo.debug) { @@ -2737,6 +2776,7 @@ class FakeFlutterDevice extends FlutterDevice { int hostVmServicePort, int ddsPort, PrintStructuredErrorLogMethod printStructuredErrorLogMethod, + bool allowExistingDdsInstance = false, }) async { } diff --git a/packages/flutter_tools/test/general.shard/terminal_handler_test.dart b/packages/flutter_tools/test/general.shard/terminal_handler_test.dart index 1183d5567c..0710fb851d 100644 --- a/packages/flutter_tools/test/general.shard/terminal_handler_test.dart +++ b/packages/flutter_tools/test/general.shard/terminal_handler_test.dart @@ -353,5 +353,6 @@ class TestRunner extends Mock implements ResidentRunner { Future attach({ Completer connectionInfoCompleter, Completer appStartedCompleter, + bool allowExistingDdsInstance = false, }) async => null; }