Wait until all scripts are parsed before recomputing metadata on a hot reload

dwds/CHANGELOG.md

@@ -1,5 +1,8 @@
 ## 24.4.1-wip
 
+- Fixed an issue where we didn't wait until all scripts were parsed before
+  recomputing metadata on a hot reload.
+
 ## 24.4.0
 
 - Added support for breakpoint registering on a hot reload with the DDC library bundle format using PausePostRequests.

dwds/lib/src/debugging/debugger.dart

@@ -56,6 +56,9 @@ class Debugger extends Domain {
 
   int _frameErrorCount = 0;
 
+  final StreamController<String> parsedScriptsController =
+      StreamController.broadcast();
+
   Debugger._(
     this._remoteDebugger,
     this._streamNotify,
@@ -489,6 +492,9 @@ class Debugger extends Domain {
     final scriptPath = _pathForChromeScript(e.script.url);
     if (scriptPath != null) {
       chromePathToRuntimeScriptId[scriptPath] = e.script.scriptId;
+      if (parsedScriptsController.hasListener) {
+        parsedScriptsController.sink.add(e.script.url);
+      }
     }
   }
 

dwds/lib/src/debugging/modules.dart

@@ -98,9 +98,6 @@ class Modules {
   ) async {
     final serverPath = await globalToolConfiguration.loadStrategy
         .serverPathForModule(entrypoint, module);
-    // TODO(srujzs): We should wait until all scripts are parsed before
-    // accessing after a hot reload. See
-    // https://github.com/dart-lang/webdev/issues/2640.
     return chromePathToRuntimeScriptId[serverPath];
   }
 

dwds/lib/src/loaders/ddc_library_bundle.dart

@@ -110,14 +110,16 @@ class DdcLibraryBundleStrategy extends LoadStrategy {
   /// ```json
   /// [
   ///   {
-  ///     "src": "<file_name>",
+  ///     "src": "<base_uri>/<file_name>",
+  ///     "module": "<module_name>",
   ///     "libraries": ["<lib1>", "<lib2>"],
   ///   },
   /// ]
   /// ```
   ///
   /// `src`: A string that corresponds to the file path containing a DDC library
   /// bundle.
+  /// `module`: The name of the library bundle in `src`.
   /// `libraries`: An array of strings containing the libraries that were
   /// compiled in `src`.
   ///

dwds/lib/src/services/chrome_proxy_service.dart

@@ -1221,15 +1221,42 @@ class ChromeProxyService implements VmServiceInterface {
   Future<void> _performClientSideHotReload(String isolateId) async {
     _logger.info('Attempting a hot reload');
 
+    final debugger = await debuggerFuture;
+    final reloadedSrcs = <String>{};
+    final computedReloadedSrcs = Completer<void>();
+    final parsedAllReloadedSrcs = Completer<void>();
+    // Wait until all the reloaded scripts are parsed before we reinitialize
+    // metadata below.
+    final parsedScriptsSubscription = debugger.parsedScriptsController.stream
+        .listen((url) {
+          computedReloadedSrcs.future.then((_) {
+            reloadedSrcs.remove(Uri.parse(url).normalizePath().path);
+            if (reloadedSrcs.isEmpty) {
+              parsedAllReloadedSrcs.complete();
+            }
+          });
+        });
+
     // Initiate a hot reload.
     _logger.info('Issuing \$dartHotReloadStartDwds request');
     final remoteObject = await inspector.jsEvaluate(
       '\$dartHotReloadStartDwds();',
       awaitPromise: true,
       returnByValue: true,
     );
-    final reloadedModulesToLibraries =
-        (remoteObject.value as Map).cast<String, List>();
+    final reloadedSrcModuleLibraries = (remoteObject.value as List).cast<Map>();
+    final reloadedModulesToLibraries = <String, List<String>>{};
+    for (final srcModuleLibrary in reloadedSrcModuleLibraries) {
+      final srcModuleLibraryCast = srcModuleLibrary.cast<String, Object>();
+      reloadedSrcs.add(
+        Uri.parse(srcModuleLibraryCast['src'] as String).normalizePath().path,
+      );
+      reloadedModulesToLibraries[srcModuleLibraryCast['module'] as String] =
+          (srcModuleLibraryCast['libraries'] as List).cast<String>();
+    }
+    computedReloadedSrcs.complete();
+    if (reloadedSrcs.isNotEmpty) await parsedAllReloadedSrcs.future;
+    await parsedScriptsSubscription.cancel();
 
     if (!pauseIsolatesOnStart) {
       // Finish hot reload immediately.

dwds/test/fixtures/context.dart

@@ -339,11 +339,12 @@ class TestContext {
             _hostname = appMetadata.hostname;
             await webRunner.run(
               frontendServerFileSystem,
-              _hostname,
-              assetServerPort,
-              filePathToServe,
+              hostname: _hostname,
+              port: assetServerPort,
+              index: filePathToServe,
               initialCompile: true,
               fullRestart: false,
+              fileServerUri: null,
             );
 
             if (testSettings.enableExpressionEvaluation) {
@@ -433,6 +434,10 @@ class TestContext {
       final appConnectionCompleter = Completer();
       final connection = ChromeConnection('localhost', debugPort);
 
+      // TODO(srujzs): In the case of the frontend server, it doesn't make sense
+      // that we initialize a new HTTP server instead of reusing the one in
+      // `TestAssetServer`. Why not just use that one? That seems to match with
+      // what Flutter tools is doing.
       _testServer = await TestServer.start(
         debugSettings: debugSettings.copyWith(
           expressionCompiler: expressionCompiler,
@@ -592,11 +597,9 @@ class TestContext {
   Future<void> recompile({required bool fullRestart}) async {
     await webRunner.run(
       frontendServerFileSystem,
-      _hostname,
-      await findUnusedPort(),
-      webCompatiblePath([project.directoryToServe, project.filePathToServe]),
       initialCompile: false,
       fullRestart: fullRestart,
+      fileServerUri: Uri.parse('http://${testServer.host}:${testServer.port}'),
     );
     return;
   }

dwds/test/hot_reload_breakpoints_test.dart

@@ -8,7 +8,6 @@
 library;
 
 import 'dart:async';
-import 'dart:io';
 
 import 'package:dwds/expression_compiler.dart';
 import 'package:test/test.dart';
@@ -207,6 +206,37 @@ void main() {
     Future<void> waitForBreakpoint() =>
         expectLater(stream, emitsThrough(_hasKind(EventKind.kPauseBreakpoint)));
 
+    test('empty hot reload keeps breakpoints', () async {
+      final genString = 'main gen0';
+
+      final bp = await addBreakpoint(
+        file: mainFile,
+        breakpointMarker: callLogMarker,
+      );
+
+      var breakpointFuture = waitForBreakpoint();
+
+      await callEvaluate();
+
+      // Should break at `callLog`.
+      await breakpointFuture;
+      await resumeAndExpectLog(genString);
+
+      await context.recompile(fullRestart: false);
+
+      await hotReloadAndHandlePausePost([
+        (file: mainFile, breakpointMarker: callLogMarker, bp: bp),
+      ]);
+
+      breakpointFuture = waitForBreakpoint();
+
+      await callEvaluate();
+
+      // Should break at `callLog`.
+      await breakpointFuture;
+      await resumeAndExpectLog(genString);
+    });
+
     test('after edit and hot reload, breakpoint is in new file', () async {
       final oldString = 'main gen0';
       final newString = 'main gen1';
@@ -359,52 +389,109 @@ void main() {
       },
     );
 
-    test(
-      'breakpoint in captured code is deleted',
-      () async {
-        var bp = await addBreakpoint(
-          file: mainFile,
-          breakpointMarker: capturedStringMarker,
-        );
+    // Test that we wait for all scripts to be parsed first before computing
+    // location metadata.
+    test('after adding many files and putting breakpoint in the last one,'
+        'breakpoint is correctly registered', () async {
+      final genLog = 'main gen0';
 
-        final oldLog = "log('\$mainValue');";
-        final newLog = "log('\${closure()}');";
-        await makeEditAndRecompile(mainFile, oldLog, newLog);
+      final bp = await addBreakpoint(
+        file: mainFile,
+        breakpointMarker: callLogMarker,
+      );
 
-        bp =
-            (await hotReloadAndHandlePausePost([
-              (file: mainFile, breakpointMarker: capturedStringMarker, bp: bp),
-            ])).first;
+      var breakpointFuture = waitForBreakpoint();
 
-        final breakpointFuture = waitForBreakpoint();
+      await callEvaluate();
 
-        await callEvaluate();
+      // Should break at `callLog`.
+      await breakpointFuture;
+      await resumeAndExpectLog(genLog);
 
-        // Should break at `capturedString`.
-        await breakpointFuture;
-        final oldCapturedString = 'captured closure gen0';
-        // Closure gets evaluated for the first time.
-        await resumeAndExpectLog(oldCapturedString);
-
-        final newCapturedString = 'captured closure gen1';
-        await makeEditAndRecompile(
-          mainFile,
-          oldCapturedString,
-          newCapturedString,
+      // Add library files, import them, but only refer to the last one in main.
+      final numFiles = 50;
+      for (var i = 1; i <= numFiles; i++) {
+        final libFile = 'library$i.dart';
+        context.addLibraryFile(
+          libFileName: libFile,
+          contents: '''String get libraryValue$i {
+            return 'lib gen$i'; // Breakpoint: libValue$i
+          }''',
         );
+        final oldImports = "import 'dart:js_interop';";
+        final newImports =
+            '$oldImports\n'
+            "import 'package:_test_hot_reload_breakpoints/$libFile';";
+        makeEdit(mainFile, oldImports, newImports);
+      }
+      final oldLog = "log('\$mainValue');";
+      final newLog = "log('\$libraryValue$numFiles');";
+      await makeEditAndRecompile(mainFile, oldLog, newLog);
 
-        await hotReloadAndHandlePausePost([
-          (file: mainFile, breakpointMarker: capturedStringMarker, bp: bp),
-        ]);
+      await hotReloadAndHandlePausePost([
+        (file: mainFile, breakpointMarker: callLogMarker, bp: bp),
+        (
+          file: 'library$numFiles.dart',
+          breakpointMarker: 'libValue$numFiles',
+          bp: null,
+        ),
+      ]);
 
-        // Breakpoint should not be hit as it's now deleted. We should also see
-        // the old string still as the closure has not been reevaluated.
-        await callEvaluateAndExpectLog(oldCapturedString);
-      },
-      // TODO(srujzs): Re-enable after
-      // https://github.com/dart-lang/webdev/issues/2640.
-      skip: Platform.isWindows,
-    );
+      breakpointFuture = waitForBreakpoint();
+
+      await callEvaluate();
+
+      // Should break at `callLog`.
+      await breakpointFuture;
+
+      breakpointFuture = waitForBreakpoint();
+
+      await resume();
+      // Should break at the breakpoint in the last file.
+      await breakpointFuture;
+      await resumeAndExpectLog('lib gen$numFiles');
+    });
+
+    test('breakpoint in captured code is deleted', () async {
+      var bp = await addBreakpoint(
+        file: mainFile,
+        breakpointMarker: capturedStringMarker,
+      );
+
+      final oldLog = "log('\$mainValue');";
+      final newLog = "log('\${closure()}');";
+      await makeEditAndRecompile(mainFile, oldLog, newLog);
+
+      bp =
+          (await hotReloadAndHandlePausePost([
+            (file: mainFile, breakpointMarker: capturedStringMarker, bp: bp),
+          ])).first;
+
+      final breakpointFuture = waitForBreakpoint();
+
+      await callEvaluate();
+
+      // Should break at `capturedString`.
+      await breakpointFuture;
+      final oldCapturedString = 'captured closure gen0';
+      // Closure gets evaluated for the first time.
+      await resumeAndExpectLog(oldCapturedString);
+
+      final newCapturedString = 'captured closure gen1';
+      await makeEditAndRecompile(
+        mainFile,
+        oldCapturedString,
+        newCapturedString,
+      );
+
+      await hotReloadAndHandlePausePost([
+        (file: mainFile, breakpointMarker: capturedStringMarker, bp: bp),
+      ]);
+
+      // Breakpoint should not be hit as it's now deleted. We should also see
+      // the old string still as the closure has not been reevaluated.
+      await callEvaluateAndExpectLog(oldCapturedString);
+    });
   }, timeout: Timeout.factor(2));
 
   group('when pause_isolates_on_start is false', () {