From ba4d63a47bfdaaba7dd62c5a0585eb4fa505c749 Mon Sep 17 00:00:00 2001 From: Jason Simmons Date: Wed, 2 Feb 2022 14:55:18 -0800 Subject: [PATCH] Exit the tool if a DevTools subprocess fails when running on a bot (#97613) --- packages/flutter_tools/lib/executable.dart | 1 + .../flutter_tools/lib/src/context_runner.dart | 1 + .../lib/src/devtools_launcher.dart | 22 ++++++++++++-- .../general.shard/devtools_launcher_test.dart | 30 +++++++++++++++++++ .../resident_devtools_handler_test.dart | 2 ++ 5 files changed, 54 insertions(+), 2 deletions(-) diff --git a/packages/flutter_tools/lib/executable.dart b/packages/flutter_tools/lib/executable.dart index a1b891d8104..01223506e40 100644 --- a/packages/flutter_tools/lib/executable.dart +++ b/packages/flutter_tools/lib/executable.dart @@ -112,6 +112,7 @@ Future main(List args) async { processManager: globals.processManager, dartExecutable: globals.artifacts.getHostArtifact(HostArtifact.engineDartBinary).path, logger: globals.logger, + botDetector: globals.botDetector, ), Logger: () { final LoggerFactory loggerFactory = LoggerFactory( diff --git a/packages/flutter_tools/lib/src/context_runner.dart b/packages/flutter_tools/lib/src/context_runner.dart index 2d8eede3c5a..9931c6dd642 100644 --- a/packages/flutter_tools/lib/src/context_runner.dart +++ b/packages/flutter_tools/lib/src/context_runner.dart @@ -218,6 +218,7 @@ Future runInContext( processManager: globals.processManager, dartExecutable: globals.artifacts.getHostArtifact(HostArtifact.engineDartBinary).path, logger: globals.logger, + botDetector: globals.botDetector, ), Doctor: () => Doctor(logger: globals.logger), DoctorValidatorsProvider: () => DoctorValidatorsProvider.defaultInstance, diff --git a/packages/flutter_tools/lib/src/devtools_launcher.dart b/packages/flutter_tools/lib/src/devtools_launcher.dart index 08e80ffa763..c7dfa0c46ac 100644 --- a/packages/flutter_tools/lib/src/devtools_launcher.dart +++ b/packages/flutter_tools/lib/src/devtools_launcher.dart @@ -9,6 +9,8 @@ import 'dart:async'; import 'package:meta/meta.dart'; import 'package:process/process.dart'; +import 'base/bot_detector.dart'; +import 'base/common.dart'; import 'base/io.dart' as io; import 'base/logger.dart'; import 'convert.dart'; @@ -21,16 +23,22 @@ class DevtoolsServerLauncher extends DevtoolsLauncher { @required ProcessManager processManager, @required String dartExecutable, @required Logger logger, + @required BotDetector botDetector, }) : _processManager = processManager, _dartExecutable = dartExecutable, - _logger = logger; + _logger = logger, + _botDetector = botDetector; final ProcessManager _processManager; final String _dartExecutable; final Logger _logger; + final BotDetector _botDetector; final Completer _processStartCompleter = Completer(); io.Process _devToolsProcess; + bool _devToolsProcessKilled = false; + @visibleForTesting + Future devToolsProcessExit; static final RegExp _serveDevToolsPattern = RegExp(r'Serving DevTools at ((http|//)[a-zA-Z0-9:/=_\-\.\[\]]+?)\.?$'); @@ -66,6 +74,16 @@ class DevtoolsServerLauncher extends DevtoolsLauncher { .transform(utf8.decoder) .transform(const LineSplitter()) .listen(_logger.printError); + + final bool runningOnBot = await _botDetector.isRunningOnBot; + devToolsProcessExit = _devToolsProcess.exitCode.then( + (int exitCode) { + if (!_devToolsProcessKilled && runningOnBot) { + throwToolExit('DevTools process failed: exitCode=$exitCode'); + } + } + ); + devToolsUrl = await completer.future; } on Exception catch (e, st) { _logger.printError('Failed to launch DevTools: $e', stackTrace: st); @@ -86,8 +104,8 @@ class DevtoolsServerLauncher extends DevtoolsLauncher { devToolsUrl = null; } if (_devToolsProcess != null) { + _devToolsProcessKilled = true; _devToolsProcess.kill(); - await _devToolsProcess.exitCode; } } } diff --git a/packages/flutter_tools/test/general.shard/devtools_launcher_test.dart b/packages/flutter_tools/test/general.shard/devtools_launcher_test.dart index b500d4a59f0..4b835cc0aa6 100644 --- a/packages/flutter_tools/test/general.shard/devtools_launcher_test.dart +++ b/packages/flutter_tools/test/general.shard/devtools_launcher_test.dart @@ -14,6 +14,7 @@ import 'package:flutter_tools/src/resident_runner.dart'; import '../src/common.dart'; import '../src/fake_process_manager.dart'; +import '../src/fakes.dart'; void main() { BufferLogger logger; @@ -29,6 +30,7 @@ void main() { final DevtoolsLauncher launcher = DevtoolsServerLauncher( dartExecutable: 'dart', logger: logger, + botDetector: const FakeBotDetector(false), processManager: FakeProcessManager.list([ FakeCommand( command: const [ @@ -52,6 +54,7 @@ void main() { final DevtoolsLauncher launcher = DevtoolsServerLauncher( dartExecutable: 'dart', logger: logger, + botDetector: const FakeBotDetector(false), processManager: FakeProcessManager.list([ FakeCommand( command: const [ @@ -91,6 +94,7 @@ void main() { final DevtoolsLauncher launcher = DevtoolsServerLauncher( dartExecutable: 'dart', logger: logger, + botDetector: const FakeBotDetector(false), processManager: processManager, ); @@ -104,6 +108,7 @@ void main() { final DevtoolsLauncher launcher = DevtoolsServerLauncher( dartExecutable: 'dart', logger: logger, + botDetector: const FakeBotDetector(false), processManager: FakeProcessManager.list([ const FakeCommand( command: [ @@ -121,4 +126,29 @@ void main() { expect(logger.errorText, contains('Failed to launch DevTools: ProcessException')); }); + + testWithoutContext('DevtoolsLauncher handles failure of DevTools process on a bot', () async { + final Completer completer = Completer(); + final DevtoolsServerLauncher launcher = DevtoolsServerLauncher( + dartExecutable: 'dart', + logger: logger, + botDetector: const FakeBotDetector(true), + processManager: FakeProcessManager.list([ + FakeCommand( + command: const [ + 'dart', + 'devtools', + '--no-launch-browser', + ], + stdout: 'Serving DevTools at http://127.0.0.1:9100\n', + completer: completer, + exitCode: 255, + ), + ]), + ); + + await launcher.launch(null); + completer.complete(); + expect(launcher.devToolsProcessExit, throwsToolExit()); + }); } diff --git a/packages/flutter_tools/test/general.shard/resident_devtools_handler_test.dart b/packages/flutter_tools/test/general.shard/resident_devtools_handler_test.dart index fd13db1c949..9e11593f00c 100644 --- a/packages/flutter_tools/test/general.shard/resident_devtools_handler_test.dart +++ b/packages/flutter_tools/test/general.shard/resident_devtools_handler_test.dart @@ -20,6 +20,7 @@ import 'package:vm_service/vm_service.dart' as vm_service; import '../src/common.dart'; import '../src/fake_process_manager.dart'; import '../src/fake_vm_services.dart'; +import '../src/fakes.dart'; final vm_service.Isolate isolate = vm_service.Isolate( id: '1', @@ -106,6 +107,7 @@ void main() { processManager: FakeProcessManager.empty(), dartExecutable: 'dart', logger: BufferLogger.test(), + botDetector: const FakeBotDetector(false), ); final ResidentDevtoolsHandler handler = FlutterResidentDevtoolsHandler( // Uses real devtools instance which should be a no-op if