From 3abd0f3fb2b630d1c7ba9beba19fb091bbff22fb Mon Sep 17 00:00:00 2001 From: Srujan Gaddam Date: Thu, 14 Aug 2025 15:41:17 -0700 Subject: [PATCH 1/2] [dwds] Fix bug where no-op hot restart doesn't cancel a subscription 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. --- dwds/CHANGELOG.md | 7 +++ dwds/lib/src/dwds_vm_client.dart | 8 ++-- .../src/services/chrome_proxy_service.dart | 22 ++++----- dwds/lib/src/version.dart | 2 +- dwds/pubspec.yaml | 2 +- dwds/test/common/hot_restart_common.dart | 36 ++++++++++++++ dwds/test/hot_reload_test.dart | 47 +++++++++++++++++-- 7 files changed, 104 insertions(+), 20 deletions(-) diff --git a/dwds/CHANGELOG.md b/dwds/CHANGELOG.md index b32fe60af..2add9e7f8 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 73c3aff72..37f24bf3d 100644 --- a/dwds/lib/src/dwds_vm_client.dart +++ b/dwds/lib/src/dwds_vm_client.dart @@ -611,20 +611,20 @@ Future> _hotRestart( globalToolConfiguration.loadStrategy is DdcLibraryBundleStrategy; final computedReloadedSrcs = Completer(); final reloadedSrcs = {}; + late StreamSubscription 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(); final debugger = await chromeProxyService.debuggerFuture; - late StreamSubscription 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 @@ Future> _hotRestart( 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 288e1ee6c..198e54155 100644 --- a/dwds/lib/src/services/chrome_proxy_service.dart +++ b/dwds/lib/src/services/chrome_proxy_service.dart @@ -1089,18 +1089,15 @@ class ChromeProxyService extends ProxyService { final parsedAllReloadedSrcs = Completer(); // Wait until all the reloaded scripts are parsed before we reinitialize // metadata below. - late StreamSubscription 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 @@ class ChromeProxyService extends ProxyService { } 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 b40640984..fa092f758 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 e3ae323a4..3b9e52480 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 504704a15..5cea54d1e 100644 --- a/dwds/test/common/hot_restart_common.dart +++ b/dwds/test/common/hot_restart_common.dart @@ -470,6 +470,42 @@ void runTests({ ), ); }); + + 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 d9eec22f9..cbf76dc5f 100644 --- a/dwds/test/hot_reload_test.dart +++ b/dwds/test/hot_reload_test.dart @@ -33,13 +33,17 @@ void main() { tearDownAll(provider.dispose); + Future recompile() async { + await context.recompile(fullRestart: false); + } + Future makeEditAndRecompile() async { context.makeEditToDartLibFile( libFileName: 'library1.dart', toReplace: originalString, replaceWith: newString, ); - await context.recompile(fullRestart: false); + await recompile(); } group('Injected client', () { @@ -64,11 +68,10 @@ void main() { 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); @@ -84,5 +87,43 @@ void main() { 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); + + final rootLib = isolate.rootLib; + await client.evaluate(isolate.id!, rootLib!.id!, 'evaluate()'); + 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); + + await client.evaluate(isolate.id!, rootLib.id!, 'evaluate()'); + 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); + + await client.evaluate(isolate.id!, rootLib.id!, 'evaluate()'); + source = await context.webDriver.pageSource; + expect(source, contains(newString)); + expect(source.contains(originalString), false); + }); }, timeout: Timeout.factor(2)); } From 9f1f120d98fda2b07b7b156064c8f3bedce6752c Mon Sep 17 00:00:00 2001 From: Srujan Gaddam Date: Thu, 14 Aug 2025 17:44:19 -0700 Subject: [PATCH 2/2] Add futures to wait for evaluate to execute --- dwds/test/hot_reload_test.dart | 24 ++++++++++++++++++++++++ fixtures/_test_hot_reload/web/main.dart | 6 ++++++ 2 files changed, 30 insertions(+) diff --git a/dwds/test/hot_reload_test.dart b/dwds/test/hot_reload_test.dart index cbf76dc5f..27c48fad4 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'; @@ -46,6 +48,20 @@ void main() { await recompile(); } + /// Wait for `evaluate` to finish executing before checking expectations by + /// checking for a log output. + Future waitForEvaluateToExecute() async { + final completer = Completer(); + 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', () { late VmService fakeClient; @@ -81,8 +97,10 @@ void main() { 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); @@ -99,8 +117,10 @@ void main() { 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); @@ -110,7 +130,9 @@ void main() { 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); @@ -120,7 +142,9 @@ void main() { 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 fb4229367..e1b1f02f8 100644 --- a/fixtures/_test_hot_reload/web/main.dart +++ b/fixtures/_test_hot_reload/web/main.dart @@ -7,11 +7,17 @@ import 'dart:js_interop'; 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() {