From edd03a1af856a794255763cb31b003886ff821e0 Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Wed, 11 Mar 2020 15:52:04 -0700 Subject: [PATCH] [flutter_tools] Update background isolates when performing hot reload/restart (#52149) When performing a hot restart, collect isolates without an attached flutter view and send a kill signal. These must have been spawned by running main, so restarting without removing them leads to isolate duplication. When performing a hot reload, ensure that we send a reloadSources command to every isolate and not just uiIsolates. --- .../lib/src/resident_runner.dart | 4 +- packages/flutter_tools/lib/src/run_hot.dart | 45 ++++---- .../general.shard/resident_runner_test.dart | 4 + .../background_isolate_test.dart | 88 ++++++++++++++ .../test_data/background_project.dart | 108 ++++++++++++++++++ 5 files changed, 224 insertions(+), 25 deletions(-) create mode 100644 packages/flutter_tools/test/integration.shard/background_isolate_test.dart create mode 100644 packages/flutter_tools/test/integration.shard/test_data/background_project.dart diff --git a/packages/flutter_tools/lib/src/resident_runner.dart b/packages/flutter_tools/lib/src/resident_runner.dart index 7011fb4fac2..6aa3530c7f8 100644 --- a/packages/flutter_tools/lib/src/resident_runner.dart +++ b/packages/flutter_tools/lib/src/resident_runner.dart @@ -283,8 +283,8 @@ class FlutterDevice { final Uri deviceEntryUri = devFS.baseUri.resolveUri(globals.fs.path.toUri(entryPath)); final Uri devicePackagesUri = devFS.baseUri.resolve('.packages'); return >>[ - for (final FlutterView view in views) - view.uiIsolate.reloadSources( + for (final Isolate isolate in vmService.vm.isolates) + isolate.reloadSources( pause: pause, rootLibUri: deviceEntryUri, packagesUri: devicePackagesUri, diff --git a/packages/flutter_tools/lib/src/run_hot.dart b/packages/flutter_tools/lib/src/run_hot.dart index 366466c15ca..22f68e22486 100644 --- a/packages/flutter_tools/lib/src/run_hot.dart +++ b/packages/flutter_tools/lib/src/run_hot.dart @@ -93,9 +93,9 @@ class HotRunner extends ResidentRunner { bool _didAttach = false; final Map> benchmarkData = >{}; - // The initial launch is from a snapshot. - bool _runningFromSnapshot = true; + DateTime firstBuildTime; + bool _shouldResetAssetDirectory = true; void _addBenchmarkData(String name, int value) { benchmarkData[name] ??= []; @@ -520,14 +520,16 @@ class HotRunner extends ResidentRunner { } } // Check if the isolate is paused and resume it. - final List> futures = >[]; + final List> operations = >[]; for (final FlutterDevice device in flutterDevices) { + final Set uiIsolates = {}; for (final FlutterView view in device.views) { if (view.uiIsolate == null) { continue; } + uiIsolates.add(view.uiIsolate); // Reload the isolate. - futures.add(view.uiIsolate.reload().then((ServiceObject _) { + operations.add(view.uiIsolate.reload().then((ServiceObject _) { final ServiceEvent pauseEvent = view.uiIsolate.pauseEvent; if ((pauseEvent != null) && pauseEvent.isPauseEvent) { // Resume the isolate so that it can be killed by the embedder. @@ -536,16 +538,22 @@ class HotRunner extends ResidentRunner { return null; })); } + // The engine handles killing and recreating isolates that it has spawned + // ("uiIsolates"). The isolates that were spawned from these uiIsolates + // will not be restared, and so they must be manually killed. + for (final Isolate isolate in device?.vmService?.vm?.isolates ?? []) { + if (!uiIsolates.contains(isolate)) { + operations.add(isolate.invokeRpcRaw('kill', params: { + 'isolateId': isolate.id, + })); + } + } } - await Future.wait(futures); + await Future.wait(operations); - // We are now running from source. - _runningFromSnapshot = false; await _launchFromDevFS(mainPath + '.dill'); restartTimer.stop(); globals.printTrace('Hot restart performed in ${getElapsedAsMilliseconds(restartTimer.elapsed)}.'); - // We are now running from sources. - _runningFromSnapshot = false; _addBenchmarkData('hotRestartMillisecondsToFrame', restartTimer.elapsed.inMilliseconds); @@ -734,10 +742,8 @@ class HotRunner extends ResidentRunner { String reason, bool pause, }) async { - final bool reloadOnTopOfSnapshot = _runningFromSnapshot; - final String progressPrefix = reloadOnTopOfSnapshot ? 'Initializing' : 'Performing'; Status status = globals.logger.startProgress( - '$progressPrefix hot reload...', + 'Performing hot reload...', timeout: timeoutConfiguration.fastOperation, progressId: 'hot.reload', ); @@ -788,13 +794,6 @@ class HotRunner extends ResidentRunner { } } - // The initial launch is from a script snapshot. When we reload from source - // on top of a script snapshot, the first reload will be a worst case reload - // because all of the sources will end up being dirty (library paths will - // change from host path to a device path). Subsequent reloads will - // not be affected, so we resume reporting reload times on the second - // reload. - bool shouldReportReloadTime = !_runningFromSnapshot; final Stopwatch reloadTimer = Stopwatch()..start(); if (!_isPaused()) { @@ -805,6 +804,7 @@ class HotRunner extends ResidentRunner { final Stopwatch devFSTimer = Stopwatch()..start(); final UpdateFSReport updatedDevFS = await _updateDevFS(); // Record time it took to synchronize to DevFS. + bool shouldReportReloadTime = true; _addBenchmarkData('hotReloadDevFSSyncMilliseconds', devFSTimer.elapsed.inMilliseconds); if (!updatedDevFS.success) { return OperationResult(1, 'DevFS synchronization failed'); @@ -819,10 +819,11 @@ class HotRunner extends ResidentRunner { ); final List> allReportsFutures = >[]; for (final FlutterDevice device in flutterDevices) { - if (_runningFromSnapshot) { + if (_shouldResetAssetDirectory) { // Asset directory has to be set only once when we switch from - // running from snapshot to running from uploaded files. + // running from bundle to uploaded files. await device.resetAssetDirectory(); + _shouldResetAssetDirectory = false; } final List>> reportFutures = device.reloadSources( entryPath, pause: pause, @@ -905,8 +906,6 @@ class HotRunner extends ResidentRunner { } await Future.wait(allDevices); - // We are now running from source. - _runningFromSnapshot = false; // Check if any isolates are paused. final List reassembleViews = []; String serviceEventKind; 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 5738dcb3072..8d35618f56a 100644 --- a/packages/flutter_tools/test/general.shard/resident_runner_test.dart +++ b/packages/flutter_tools/test/general.shard/resident_runner_test.dart @@ -92,6 +92,9 @@ void main() { ]); when(mockFlutterDevice.device).thenReturn(mockDevice); when(mockFlutterView.uiIsolate).thenReturn(mockIsolate); + final MockVM mockVM = MockVM(); + when(mockVMService.vm).thenReturn(mockVM); + when(mockVM.isolates).thenReturn([mockIsolate]); when(mockFlutterView.runFromSource(any, any, any)).thenAnswer((Invocation invocation) async {}); when(mockFlutterDevice.stopEchoingDeviceLog()).thenAnswer((Invocation invocation) async { }); when(mockFlutterDevice.observatoryUris).thenAnswer((_) => Stream.value(testUri)); @@ -744,6 +747,7 @@ class MockDevicePortForwarder extends Mock implements DevicePortForwarder {} class MockUsage extends Mock implements Usage {} class MockProcessManager extends Mock implements ProcessManager {} class MockServiceEvent extends Mock implements ServiceEvent {} +class MockVM extends Mock implements VM {} class TestFlutterDevice extends FlutterDevice { TestFlutterDevice(Device device, this.views, { Stream observatoryUris }) : super(device, buildInfo: BuildInfo.debug) { diff --git a/packages/flutter_tools/test/integration.shard/background_isolate_test.dart b/packages/flutter_tools/test/integration.shard/background_isolate_test.dart new file mode 100644 index 00000000000..2546dbe9938 --- /dev/null +++ b/packages/flutter_tools/test/integration.shard/background_isolate_test.dart @@ -0,0 +1,88 @@ +// 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:file/file.dart'; +import 'package:flutter_tools/src/base/file_system.dart'; + +import '../src/common.dart'; +import 'test_data/background_project.dart'; +import 'test_driver.dart'; +import 'test_utils.dart'; + +void main() { + Directory tempDir; + + setUp(() async { + tempDir = createResolvedTempDirectorySync('hot_reload_test.'); + }); + + tearDown(() async { + tryToDelete(tempDir); + }); + + test('Hot restart kills background isolates', () async { + final BackgroundProject project = BackgroundProject(); + await project.setUpIn(tempDir); + final FlutterRunTestDriver flutter = FlutterRunTestDriver(tempDir); + + const String newBackgroundMessage = 'New Background'; + final Completer sawForgroundMessage = Completer.sync(); + final Completer sawBackgroundMessage = Completer.sync(); + final Completer sawNewBackgroundMessage = Completer.sync(); + final StreamSubscription subscription = flutter.stdout.listen((String line) { + print('[LOG]:"$line"'); + if (line.contains('Main thread') && !sawForgroundMessage.isCompleted) { + sawForgroundMessage.complete(); + } + if (line.contains('Isolate thread')) { + sawBackgroundMessage.complete(); + } + if (line.contains(newBackgroundMessage)) { + sawNewBackgroundMessage.complete(); + } + }, + ); + await flutter.run(); + await sawForgroundMessage.future; + await sawBackgroundMessage.future; + + project.updateTestIsolatePhrase(newBackgroundMessage); + await flutter.hotRestart(); + await sawBackgroundMessage.future; + // Wait a tiny amount of time in case we did not kill the background isolate. + await Future.delayed(const Duration(milliseconds: 10)); + await subscription.cancel(); + await flutter?.stop(); + }); + + test('Hot reload updates background isolates', () async { + final RepeatingBackgroundProject project = RepeatingBackgroundProject(); + await project.setUpIn(tempDir); + final FlutterRunTestDriver flutter = FlutterRunTestDriver(tempDir); + + const String newBackgroundMessage = 'New Background'; + final Completer sawBackgroundMessage = Completer.sync(); + final Completer sawNewBackgroundMessage = Completer.sync(); + final StreamSubscription subscription = flutter.stdout.listen((String line) { + print('[LOG]:"$line"'); + if (line.contains('Isolate thread') && !sawBackgroundMessage.isCompleted) { + sawBackgroundMessage.complete(); + } + if (line.contains(newBackgroundMessage) && !sawNewBackgroundMessage.isCompleted) { + sawNewBackgroundMessage.complete(); + } + }, + ); + await flutter.run(); + await sawBackgroundMessage.future; + + project.updateTestIsolatePhrase(newBackgroundMessage); + await flutter.hotReload(); + await sawNewBackgroundMessage.future; + await subscription.cancel(); + await flutter?.stop(); + }); +} diff --git a/packages/flutter_tools/test/integration.shard/test_data/background_project.dart b/packages/flutter_tools/test/integration.shard/test_data/background_project.dart new file mode 100644 index 00000000000..9b738f1527f --- /dev/null +++ b/packages/flutter_tools/test/integration.shard/test_data/background_project.dart @@ -0,0 +1,108 @@ +// 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 'package:flutter_tools/src/globals.dart' as globals; + +import '../test_utils.dart'; +import 'project.dart'; + +/// Spawns a background isolate that prints a debug message. +class BackgroundProject extends Project { + + @override + final String pubspec = ''' + name: test + environment: + sdk: ">=2.0.0-dev.68.0 <3.0.0" + + dependencies: + flutter: + sdk: flutter + '''; + + @override + final String main = r''' + import 'dart:async'; + import 'dart:isolate'; + + import 'package:flutter/widgets.dart'; + import 'package:flutter/material.dart'; + + void main() { + Isolate.spawn(background, null, debugName: 'background'); + TestMain(); + } + + void background(void message) { + TestIsolate(); + } + + class TestMain { + TestMain() { + debugPrint('Main thread'); + } + } + + class TestIsolate { + TestIsolate() { + debugPrint('Isolate thread'); + } + } + '''; + + void updateTestIsolatePhrase(String message) { + final String newMainContents = main.replaceFirst('Isolate thread', message); + writeFile(globals.fs.path.join(dir.path, 'lib', 'main.dart'), newMainContents); + } +} + +// Spawns a background isolate that repeats a message. +class RepeatingBackgroundProject extends Project { + + @override + final String pubspec = ''' + name: test + environment: + sdk: ">=2.0.0-dev.68.0 <3.0.0" + + dependencies: + flutter: + sdk: flutter + '''; + + @override + final String main = r''' + import 'dart:async'; + import 'dart:isolate'; + + import 'package:flutter/widgets.dart'; + import 'package:flutter/material.dart'; + + void main() { + Isolate.spawn(background, null, debugName: 'background'); + TestMain(); + } + + void background(void message) { + Timer.periodic(const Duration(milliseconds: 500), (Timer timer) => TestIsolate()); + } + + class TestMain { + TestMain() { + debugPrint('Main thread'); + } + } + + class TestIsolate { + TestIsolate() { + debugPrint('Isolate thread'); + } + } + '''; + + void updateTestIsolatePhrase(String message) { + final String newMainContents = main.replaceFirst('Isolate thread', message); + writeFile(globals.fs.path.join(dir.path, 'lib', 'main.dart'), newMainContents); + } +}