[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.

Author: srujzs

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.

dwds/lib/src/dwds_vm_client.dart

@@ -611,20 +611,20 @@ Future<Map<String, dynamic>> _hotRestart(
         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 @@ Future<Map<String, dynamic>> _hotRestart(
         chromeProxyService.allowedToCreateIsolate.complete();
       }
       computedReloadedSrcs.complete();
+      await chromeProxyService.allowedToCreateIsolate.future;
+      await parsedScriptsSubscription.cancel();
     } else {
       assert(remoteObject.value == null);
     }

dwds/lib/src/services/chrome_proxy_service.dart

@@ -1089,18 +1089,15 @@ class ChromeProxyService extends ProxyService {
     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 @@ class ChromeProxyService extends ProxyService {
     }
     computedReloadedSrcs.complete();
     if (reloadedSrcs.isNotEmpty) await parsedAllReloadedSrcs.future;
+    await parsedScriptsSubscription.cancel();
 
     if (!pauseIsolatesOnStart) {
       // Finish hot reload immediately.

dwds/lib/src/version.dart

@@ -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

dwds/pubspec.yaml

@@ -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(

dwds/test/common/hot_restart_common.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 @@ void main() {
 
   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 @@ 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);
 
@@ -78,8 +97,54 @@ 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);
+    });
+
+    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);

dwds/test/hot_reload_test.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() {