[dwds] Fix bug where no-op hot restart doesn't cancel a subscription and publish 25.0.1 (#2668)
In hot restart, we wait for all sources to be parsed before continuing
to create the isolate. In order to do so, we listen on parsed sources
by registering a subscription. In the case where we have no sources
to restart, we don't cancel the subscription. This results in an issue
where on the next hot restart that contains changes, the old
subscription is triggered, potentially completing an already completed
completer. Instead, we should always cancel the subscription. Hot reload
code is changed as well to do the same.
Additional checks are added to check if the completer is completed
before we complete again. While this is not needed for this issue,
it is possible other sources can be downloaded by the app, which may
trigger the function in the listener, which could potentially try and
complete the completed completer.
Tests are added for both hot restart and hot reload to check that
alternating empty hot restarts and non-empty hot restarts work.
diff --git a/dwds/CHANGELOG.md b/dwds/CHANGELOG.md
index b32fe60..2add9e7 100644
--- a/dwds/CHANGELOG.md
+++ b/dwds/CHANGELOG.md
@@ -1,3 +1,10 @@
+## 25.0.1
+
+### Bug Fixes:
+
+- Fix issue in hot restart where a hot restart with no changes followed by one
+ with changes, a `Future` was completed again, resulting in a crash.
+
## 25.0.0
- Implemented hot restart over websockets with multi window support.
diff --git a/dwds/lib/src/dwds_vm_client.dart b/dwds/lib/src/dwds_vm_client.dart
index 73c3aff..37f24bf 100644
--- a/dwds/lib/src/dwds_vm_client.dart
+++ b/dwds/lib/src/dwds_vm_client.dart
@@ -611,20 +611,20 @@
globalToolConfiguration.loadStrategy is DdcLibraryBundleStrategy;
final computedReloadedSrcs = Completer<void>();
final reloadedSrcs = <String>{};
+ late StreamSubscription<String> parsedScriptsSubscription;
if (isDdcLibraryBundle) {
// Injected client should send a request to recreate the isolate after the
// hot restart. The creation of the isolate should in turn wait until all
// scripts are parsed.
chromeProxyService.allowedToCreateIsolate = Completer<void>();
final debugger = await chromeProxyService.debuggerFuture;
- late StreamSubscription<String> parsedScriptsSubscription;
parsedScriptsSubscription = debugger.parsedScriptsController.stream
.listen((url) {
computedReloadedSrcs.future.then((_) async {
reloadedSrcs.remove(Uri.parse(url).normalizePath().path);
- if (reloadedSrcs.isEmpty) {
+ if (reloadedSrcs.isEmpty &&
+ !chromeProxyService.allowedToCreateIsolate.isCompleted) {
chromeProxyService.allowedToCreateIsolate.complete();
- await parsedScriptsSubscription.cancel();
}
});
});
@@ -648,6 +648,8 @@
chromeProxyService.allowedToCreateIsolate.complete();
}
computedReloadedSrcs.complete();
+ await chromeProxyService.allowedToCreateIsolate.future;
+ await parsedScriptsSubscription.cancel();
} else {
assert(remoteObject.value == null);
}
diff --git a/dwds/lib/src/services/chrome_proxy_service.dart b/dwds/lib/src/services/chrome_proxy_service.dart
index 288e1ee..198e541 100644
--- a/dwds/lib/src/services/chrome_proxy_service.dart
+++ b/dwds/lib/src/services/chrome_proxy_service.dart
@@ -1089,18 +1089,15 @@
final parsedAllReloadedSrcs = Completer<void>();
// Wait until all the reloaded scripts are parsed before we reinitialize
// metadata below.
- late StreamSubscription<String> parsedScriptsSubscription;
- parsedScriptsSubscription = debugger.parsedScriptsController.stream.listen((
- url,
- ) {
- computedReloadedSrcs.future.then((_) async {
- reloadedSrcs.remove(Uri.parse(url).normalizePath().path);
- if (reloadedSrcs.isEmpty) {
- parsedAllReloadedSrcs.complete();
- await parsedScriptsSubscription.cancel();
- }
- });
- });
+ final parsedScriptsSubscription = debugger.parsedScriptsController.stream
+ .listen((url) {
+ computedReloadedSrcs.future.then((_) {
+ reloadedSrcs.remove(Uri.parse(url).normalizePath().path);
+ if (reloadedSrcs.isEmpty && !parsedAllReloadedSrcs.isCompleted) {
+ parsedAllReloadedSrcs.complete();
+ }
+ });
+ });
// Initiate a hot reload.
_logger.info('Issuing \$dartHotReloadStartDwds request');
@@ -1121,6 +1118,7 @@
}
computedReloadedSrcs.complete();
if (reloadedSrcs.isNotEmpty) await parsedAllReloadedSrcs.future;
+ await parsedScriptsSubscription.cancel();
if (!pauseIsolatesOnStart) {
// Finish hot reload immediately.
diff --git a/dwds/lib/src/version.dart b/dwds/lib/src/version.dart
index b406409..fa092f7 100644
--- a/dwds/lib/src/version.dart
+++ b/dwds/lib/src/version.dart
@@ -1,2 +1,2 @@
// Generated code. Do not modify.
-const packageVersion = '25.0.0';
+const packageVersion = '25.0.1';
diff --git a/dwds/pubspec.yaml b/dwds/pubspec.yaml
index e3ae323..3b9e524 100644
--- a/dwds/pubspec.yaml
+++ b/dwds/pubspec.yaml
@@ -1,6 +1,6 @@
name: dwds
# Every time this changes you need to run `dart run build_runner build`.
-version: 25.0.0
+version: 25.0.1
description: >-
A service that proxies between the Chrome debug protocol and the Dart VM
diff --git a/dwds/test/common/hot_restart_common.dart b/dwds/test/common/hot_restart_common.dart
index 504704a..5cea54d 100644
--- a/dwds/test/common/hot_restart_common.dart
+++ b/dwds/test/common/hot_restart_common.dart
@@ -470,6 +470,42 @@
),
);
});
+
+ test('can hot restart with no changes, hot restart with changes, and '
+ 'hot restart again with no changes', () async {
+ // Empty hot restart.
+ var mainDone = waitForMainToExecute();
+ await recompile();
+ final hotRestart = context.getRegisteredServiceExtension('hotRestart');
+ await fakeClient.callServiceExtension(hotRestart!);
+
+ await mainDone;
+ var source = await context.webDriver.pageSource;
+ expect(source.contains(originalString), isTrue);
+ expect(source.contains(newString), isFalse);
+
+ // Hot restart.
+ mainDone = waitForMainToExecute();
+ await makeEditAndRecompile();
+ await fakeClient.callServiceExtension(hotRestart);
+
+ await mainDone;
+ source = await context.webDriver.pageSource;
+ // Main is re-invoked which shouldn't clear the state.
+ expect(source.contains(originalString), isTrue);
+ expect(source.contains(newString), isTrue);
+
+ // Empty hot restart.
+ mainDone = waitForMainToExecute();
+ await recompile();
+ await fakeClient.callServiceExtension(hotRestart);
+
+ await mainDone;
+ source = await context.webDriver.pageSource;
+ expect(source.contains(originalString), isTrue);
+ // `newString` should now exist twice in the source.
+ expect(source.contains(RegExp('$newString.*$newString')), isTrue);
+ });
}, timeout: Timeout.factor(2));
group(
diff --git a/dwds/test/hot_reload_test.dart b/dwds/test/hot_reload_test.dart
index d9eec22..27c48fa 100644
--- a/dwds/test/hot_reload_test.dart
+++ b/dwds/test/hot_reload_test.dart
@@ -7,6 +7,8 @@
@Timeout(Duration(minutes: 5))
library;
+import 'dart:async';
+
import 'package:dwds/expression_compiler.dart';
import 'package:test/test.dart';
import 'package:test_common/logging.dart';
@@ -33,13 +35,31 @@
tearDownAll(provider.dispose);
+ Future<void> recompile() async {
+ await context.recompile(fullRestart: false);
+ }
+
Future<void> makeEditAndRecompile() async {
context.makeEditToDartLibFile(
libFileName: 'library1.dart',
toReplace: originalString,
replaceWith: newString,
);
- await context.recompile(fullRestart: false);
+ await recompile();
+ }
+
+ /// Wait for `evaluate` to finish executing before checking expectations by
+ /// checking for a log output.
+ Future<void> waitForEvaluateToExecute() async {
+ final completer = Completer<void>();
+ final expectedString = 'evaluate executed';
+ final subscription = context.webkitDebugger.onConsoleAPICalled.listen((e) {
+ if (e.args.first.value == expectedString) {
+ completer.complete();
+ }
+ });
+ await completer.future;
+ await subscription.cancel();
}
group('Injected client', () {
@@ -64,11 +84,10 @@
test('can hot reload', () async {
final client = context.debugConnection.vmService;
- await makeEditAndRecompile();
+ await makeEditAndRecompile();
final vm = await client.getVM();
final isolate = await client.getIsolate(vm.isolates!.first.id!);
-
final report = await fakeClient.reloadSources(isolate.id!);
expect(report.success, true);
@@ -78,8 +97,54 @@
expect(source, contains(originalString));
expect(source.contains(newString), false);
+ final evaluateDone = waitForEvaluateToExecute();
final rootLib = isolate.rootLib;
await client.evaluate(isolate.id!, rootLib!.id!, 'evaluate()');
+ await evaluateDone;
+ source = await context.webDriver.pageSource;
+ expect(source, contains(newString));
+ expect(source.contains(originalString), false);
+ });
+
+ test('can hot reload with no changes, hot reload with changes, and '
+ 'hot reload again with no changes', () async {
+ final client = context.debugConnection.vmService;
+
+ // Empty hot reload,
+ await recompile();
+ final vm = await client.getVM();
+ final isolate = await client.getIsolate(vm.isolates!.first.id!);
+ var report = await fakeClient.reloadSources(isolate.id!);
+ expect(report.success, true);
+
+ var evaluateDone = waitForEvaluateToExecute();
+ final rootLib = isolate.rootLib;
+ await client.evaluate(isolate.id!, rootLib!.id!, 'evaluate()');
+ await evaluateDone;
+ var source = await context.webDriver.pageSource;
+ expect(source, contains(originalString));
+ expect(source.contains(newString), false);
+
+ // Hot reload.
+ await makeEditAndRecompile();
+ report = await fakeClient.reloadSources(isolate.id!);
+ expect(report.success, true);
+
+ evaluateDone = waitForEvaluateToExecute();
+ await client.evaluate(isolate.id!, rootLib.id!, 'evaluate()');
+ await evaluateDone;
+ source = await context.webDriver.pageSource;
+ expect(source, contains(newString));
+ expect(source.contains(originalString), false);
+
+ // Empty hot reload.
+ await recompile();
+ report = await fakeClient.reloadSources(isolate.id!);
+ expect(report.success, true);
+
+ evaluateDone = waitForEvaluateToExecute();
+ await client.evaluate(isolate.id!, rootLib.id!, 'evaluate()');
+ await evaluateDone;
source = await context.webDriver.pageSource;
expect(source, contains(newString));
expect(source.contains(originalString), false);
diff --git a/fixtures/_test_hot_reload/web/main.dart b/fixtures/_test_hot_reload/web/main.dart
index fb42293..e1b1f02 100644
--- a/fixtures/_test_hot_reload/web/main.dart
+++ b/fixtures/_test_hot_reload/web/main.dart
@@ -7,11 +7,17 @@
import 'package:_test_hot_reload/library1.dart';
+@JS('console.log')
+external void log(String _);
+
@JS('document.body.innerHTML')
external set innerHtml(String html);
void evaluate() {
innerHtml = 'Program is running!\n $reloadValue}\n';
+
+ // Wait for this print statement so that we know evaluate is done executing.
+ log('evaluate executed');
}
void main() {
