From 5ea2be69ca071c2c267b7e2889727e7b703e9f04 Mon Sep 17 00:00:00 2001 From: Ben Konyi Date: Wed, 28 Jun 2023 00:16:13 +0530 Subject: [PATCH] Reland "Fix issue where DevTools would not be immediately available when using --start-paused (#126698)" (#129368) **Original Description:** > Service extensions are unable to handle requests when the isolate they were registered on is paused. The DevTools launcher logic was waiting for some service extension invocations to complete before advertising the already active DevTools instance, but when --start-paused was provided these requests would never complete, preventing users from using DevTools to resume the paused isolate. > > Fixes https://github.com/flutter/flutter/issues/126691 **Additional changes in this PR:** The failures listed in https://github.com/flutter/flutter/pull/128117 appear to be related to a shutdown race. It's possible for the test to complete while the tool is in the process of starting and advertising DevTools, so we need to perform a check of `_shutdown` in `FlutterResidentDevtoolsHandler` before advertising DevTools. Before the original fix, this check was being performed immediately after invoking the service extensions, which creates an asynchronous gap in execution. With #126698, the callsite of the service extensions was moved and the `_shutdown` check wasn't, allowing for the tool to attempt to advertise DevTools after the DevTools server had been cleaned up. --------- Co-authored-by: Zachary Anderson --- .../lib/src/resident_devtools_handler.dart | 52 +++++++++++++----- packages/flutter_tools/lib/src/run_cold.dart | 2 + packages/flutter_tools/lib/src/run_hot.dart | 1 + .../resident_devtools_handler_test.dart | 4 +- .../integration.shard/devtools_uri_test.dart | 53 +++++++++++++++++++ 5 files changed, 98 insertions(+), 14 deletions(-) create mode 100644 packages/flutter_tools/test/integration.shard/devtools_uri_test.dart diff --git a/packages/flutter_tools/lib/src/resident_devtools_handler.dart b/packages/flutter_tools/lib/src/resident_devtools_handler.dart index 2dc0aaef1ae..3b7521480fa 100644 --- a/packages/flutter_tools/lib/src/resident_devtools_handler.dart +++ b/packages/flutter_tools/lib/src/resident_devtools_handler.dart @@ -35,6 +35,7 @@ abstract class ResidentDevtoolsHandler { Future serveAndAnnounceDevTools({ Uri? devToolsServerAddress, required List flutterDevices, + bool isStartPaused = false, }); bool launchDevToolsInBrowser({required List flutterDevices}); @@ -71,6 +72,7 @@ class FlutterResidentDevtoolsHandler implements ResidentDevtoolsHandler { Future serveAndAnnounceDevTools({ Uri? devToolsServerAddress, required List flutterDevices, + bool isStartPaused = false, }) async { assert(!_readyToAnnounce); if (!_residentRunner.supportsServiceProtocol || _devToolsLauncher == null) { @@ -88,20 +90,10 @@ class FlutterResidentDevtoolsHandler implements ResidentDevtoolsHandler { assert(!_readyToAnnounce); return; } - final List devicesWithExtension = await _devicesWithExtensions(flutterDevices); - await _maybeCallDevToolsUriServiceExtension(devicesWithExtension); - await _callConnectedVmServiceUriExtension(devicesWithExtension); - - if (_shutdown) { - // If we're shutting down, no point reporting the debugger list. - return; - } - _readyToAnnounce = true; - assert(_devToolsLauncher!.activeDevToolsServer != null); final Uri? devToolsUrl = _devToolsLauncher!.devToolsUrl; if (devToolsUrl != null) { - for (final FlutterDevice? device in devicesWithExtension) { + for (final FlutterDevice? device in flutterDevices) { if (device == null) { continue; } @@ -111,11 +103,43 @@ class FlutterResidentDevtoolsHandler implements ResidentDevtoolsHandler { } } + Future callServiceExtensions() async { + final List devicesWithExtension = await _devicesWithExtensions(flutterDevices); + await Future.wait( + >[ + _maybeCallDevToolsUriServiceExtension(devicesWithExtension), + _callConnectedVmServiceUriExtension(devicesWithExtension) + ] + ); + } + + // If the application is starting paused, we can't invoke service extensions + // as they're handled on the target app's paused isolate. Since invoking + // service extensions will block in this situation, we should wait to invoke + // them until after we've output the DevTools connection details. + if (!isStartPaused) { + await callServiceExtensions(); + } + + // This check needs to happen after the possible asynchronous call above, + // otherwise a shutdown event might be missed and the DevTools launcher may + // no longer be initialized. + if (_shutdown) { + // If we're shutting down, no point reporting the debugger list. + return; + } + + _readyToAnnounce = true; + assert(_devToolsLauncher!.activeDevToolsServer != null); if (_residentRunner.reportedDebuggers) { // Since the DevTools only just became available, we haven't had a chance to // report their URLs yet. Do so now. _residentRunner.printDebuggerList(includeVmService: false); } + + if (isStartPaused) { + await callServiceExtensions(); + } } // This must be guaranteed not to return a Future that fails. @@ -295,7 +319,11 @@ class NoOpDevtoolsHandler implements ResidentDevtoolsHandler { } @override - Future serveAndAnnounceDevTools({Uri? devToolsServerAddress, List? flutterDevices}) async { + Future serveAndAnnounceDevTools({ + Uri? devToolsServerAddress, + List? flutterDevices, + bool isStartPaused = false, + }) async { return; } diff --git a/packages/flutter_tools/lib/src/run_cold.dart b/packages/flutter_tools/lib/src/run_cold.dart index 88c3fd68e9c..cbd83bed033 100644 --- a/packages/flutter_tools/lib/src/run_cold.dart +++ b/packages/flutter_tools/lib/src/run_cold.dart @@ -86,6 +86,7 @@ class ColdRunner extends ResidentRunner { unawaited(residentDevtoolsHandler!.serveAndAnnounceDevTools( devToolsServerAddress: debuggingOptions.devToolsServerAddress, flutterDevices: flutterDevices, + isStartPaused: debuggingOptions.startPaused, )); } if (debuggingOptions.serveObservatory) { @@ -173,6 +174,7 @@ class ColdRunner extends ResidentRunner { unawaited(residentDevtoolsHandler!.serveAndAnnounceDevTools( devToolsServerAddress: debuggingOptions.devToolsServerAddress, flutterDevices: flutterDevices, + isStartPaused: debuggingOptions.startPaused, )); } if (debuggingOptions.serveObservatory) { diff --git a/packages/flutter_tools/lib/src/run_hot.dart b/packages/flutter_tools/lib/src/run_hot.dart index 124ec827bd9..7d8c77ac02b 100644 --- a/packages/flutter_tools/lib/src/run_hot.dart +++ b/packages/flutter_tools/lib/src/run_hot.dart @@ -249,6 +249,7 @@ class HotRunner extends ResidentRunner { unawaited(residentDevtoolsHandler!.serveAndAnnounceDevTools( devToolsServerAddress: debuggingOptions.devToolsServerAddress, flutterDevices: flutterDevices, + isStartPaused: debuggingOptions.startPaused, )); } 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 8c36d4e828b..56d933291e8 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 @@ -156,6 +156,7 @@ void main() { }, ), listViews, + listViews, const FakeVmServiceRequest( method: 'ext.flutter.activeDevToolsServerAddress', args: { @@ -163,7 +164,6 @@ void main() { 'value': 'http://localhost:8080', }, ), - listViews, const FakeVmServiceRequest( method: 'ext.flutter.connectedVmServiceUri', args: { @@ -314,6 +314,7 @@ void main() { }, ), listViews, + listViews, const FakeVmServiceRequest( method: 'ext.flutter.activeDevToolsServerAddress', args: { @@ -321,7 +322,6 @@ void main() { 'value': 'http://localhost:8080', }, ), - listViews, const FakeVmServiceRequest( method: 'ext.flutter.connectedVmServiceUri', args: { diff --git a/packages/flutter_tools/test/integration.shard/devtools_uri_test.dart b/packages/flutter_tools/test/integration.shard/devtools_uri_test.dart new file mode 100644 index 00000000000..af15861554e --- /dev/null +++ b/packages/flutter_tools/test/integration.shard/devtools_uri_test.dart @@ -0,0 +1,53 @@ +// Copyright 2014 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'dart:async'; + +import 'package:flutter_tools/src/base/file_system.dart'; +import 'package:flutter_tools/src/base/io.dart'; +import 'package:flutter_tools/src/convert.dart'; + +import '../src/common.dart'; +import 'test_data/basic_project.dart'; +import 'test_utils.dart'; + +void main() { + late Directory tempDir; + final BasicProject project = BasicProject(); + + setUp(() async { + tempDir = createResolvedTempDirectorySync('run_test.'); + await project.setUpIn(tempDir); + }); + + tearDown(() async { + tryToDelete(tempDir); + }); + + // Regression test for https://github.com/flutter/flutter/issues/126691 + testWithoutContext('flutter run --start-paused prints DevTools URI', () async { + final Completer completer = Completer(); + const String matcher = 'The Flutter DevTools debugger and profiler on'; + + final String flutterBin = fileSystem.path.join(getFlutterRoot(), 'bin', 'flutter'); + final Process process = await processManager.start([ + flutterBin, + 'run', + '--start-paused', + '-d', + 'flutter-tester', + ], workingDirectory: tempDir.path); + + final StreamSubscription sub; + sub = process.stdout.transform(utf8.decoder).listen((String message) { + if (message.contains(matcher)) { + completer.complete(); + } + }); + await completer.future; + await sub.cancel(); + process.kill(); + await process.exitCode; + }); +}