Fix race condition on simultaneous hot restarts (#1870)

* Wait for isolate to be destroyed before creating a new one

* Update changelog

* Fix race condition on simultaneous hot restarts

- Fix race condition on running two `createIsolate` calls simultaneously.
  - Run `destroyIsolate` followed by `createIsolate` an atomic operation.
  - Make debugger API that depend on isolate running wait for the start
    of the app.
  - Make debugger API throw if the isolate exits while the API is waiting.
- Fix exception on uninitialized `DwdsStats._devToolsStart` when using
  an observatory uri to connect to the dwds VM service.
- Remove unnecessary async keywords from synchronoous debugger API.

Closes: ihttps://github.com//issues/1869

* Fix race condition on simultaneous hot restarts

- Fix race condition on running two `createIsolate` calls simultaneously.
  - Run `destroyIsolate` followed by `createIsolate` an atomic operation.
  - Make debugger API that depend on isolate running wait for the start
    of the app.
  - Make debugger API throw if the isolate exits while the API is waiting.
- Fix exception on uninitialized `DwdsStats._devToolsStart` when using
  an observatory uri to connect to the dwds VM service.
- Remove unnecessary async keywords from synchronoous debugger API.

Closes: ihttps://github.com//issues/1869

* Keep race condition changes only

* Revert unneded changes again

* Merged with master

* Update changelog, pubspec, and build

* Make hot restart atomic changes only

* Address CR Comments

Author: Anna Gringauze

dwds/CHANGELOG.md

@@ -1,26 +1,28 @@
-## 16.0.2-dev
+## 17.0.0-dev
 
 - Include debug information in the event sent from the injected client to the
   Dart Debug Extension notifying that the Dart app is ready.
-- Include an optional param to `Dwds.start` to indicate whether it is running
-  internally or externally.
 - Fix null cast error on expression evaluations after dwds fails to find class
   metadata.
 - Include the entire exception description up to the stacktrace in
   `mapExceptionStackTrace`.
 - Allow enabling experiments in the expression compiler service.
-- Include an optional param to `Dwds.start` to indicate whether it a Flutter app
-  or not.
 - Pre-warm expression compiler cache to speed up Flutter Inspector loading.
-- Remove `ChromeProxyService.setExceptionPauseMode()`.
 - Display full error on failure to start DDS.
 - Fix crash on processing DevTools event when starting DevTools from DevTools
   uri.
 - Prepare or Dart 3 alpha breaking changes:
   - Move weak null safety tests to special branch of `build_web_compilers`.
   - Do not pass `--(no)-sound-null-safety` flag to build daemon.
 - Add back `ChromeProxyService.setExceptionPauseMode()` without override.
+- Make hot restart atomic to prevent races on simultaneous execution.
 
+**Breaking changes**
+- Include an optional param to `Dwds.start` to indicate whether it is running
+  internally or externally.
+- Include an optional param to `Dwds.start` to indicate whether it a Flutter app
+  or not.
+- Remove deprecated `ChromeProxyService.setExceptionPauseMode()`.
 
 ## 16.0.1
 

dwds/lib/src/debugging/debugger.dart

@@ -5,8 +5,8 @@
 import 'dart:async';
 import 'dart:math' as math;
 
+import 'package:dwds/src/utilities/synchronized.dart';
 import 'package:logging/logging.dart';
-import 'package:pool/pool.dart';
 import 'package:vm_service/vm_service.dart';
 import 'package:webkit_inspection_protocol/webkit_inspection_protocol.dart'
     hide StackTrace;
@@ -779,7 +779,7 @@ class _Breakpoints extends Domain {
 
   final _bpByDartId = <String, Future<Breakpoint>>{};
 
-  final _pool = Pool(1);
+  final _queue = AtomicQueue();
 
   final Locations locations;
   final RemoteDebugger remoteDebugger;
@@ -868,7 +868,7 @@ class _Breakpoints extends Domain {
     final urlRegex = '.*${location.jsLocation.module}.*';
     // Prevent `Aww, snap!` errors when setting multiple breakpoints
     // simultaneously by serializing the requests.
-    return _pool.withResource(() async {
+    return _queue.run(() async {
       final breakPointId = await sendCommandAndValidateResult<String>(
           remoteDebugger,
           method: 'Debugger.setBreakpointByUrl',

dwds/lib/src/debugging/frame_computer.dart

@@ -2,7 +2,7 @@
 // for details. 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:pool/pool.dart';
+import 'package:dwds/src/utilities/synchronized.dart';
 import 'package:vm_service/vm_service.dart';
 import 'package:webkit_inspection_protocol/webkit_inspection_protocol.dart';
 
@@ -11,9 +11,9 @@ import 'debugger.dart';
 class FrameComputer {
   final Debugger debugger;
 
-  // To ensure that the frames are computed only once, we use a pool to guard
-  // the work. Frames are computed sequentially.
-  final _pool = Pool(1);
+  // To ensure that the frames are computed only once, we use an atomic queue
+  // to guard the work. Frames are computed sequentially.
+  final _queue = AtomicQueue();
 
   final List<WipCallFrame> _callFrames;
   final List<Frame> _computedFrames = [];
@@ -36,7 +36,7 @@ class FrameComputer {
   /// Translates Chrome callFrames contained in [DebuggerPausedEvent] into Dart
   /// [Frame]s.
   Future<List<Frame>> calculateFrames({int? limit}) async {
-    return _pool.withResource(() async {
+    return _queue.run(() async {
       if (limit != null && _computedFrames.length >= limit) {
         return _computedFrames.take(limit).toList();
       }

dwds/lib/src/dwds_vm_client.dart

@@ -5,6 +5,7 @@
 import 'dart:async';
 import 'dart:convert';
 
+import 'package:dwds/src/utilities/synchronized.dart';
 import 'package:logging/logging.dart';
 import 'package:uuid/uuid.dart';
 import 'package:vm_service/vm_service.dart';
@@ -32,6 +33,9 @@ class DwdsVmClient {
   /// All subsequent calls to [close] will return this future.
   Future<void>? _closed;
 
+  /// Synchronizes hot restarts to avoid races.
+  final _hotRestartQueue = AtomicQueue();
+
   DwdsVmClient(this.client, this._requestController, this._responseController);
 
   Future<void> close() => _closed ??= () async {
@@ -60,6 +64,9 @@ class DwdsVmClient {
     final chromeProxyService =
         debugService.chromeProxyService as ChromeProxyService;
 
+    final dwdsVmClient =
+        DwdsVmClient(client, requestController, responseController);
+
     // Register '_flutter.listViews' method on the chrome proxy service vm.
     // In native world, this method is provided by the engine, but the web
     // engine is not aware of the VM uri or the isolates.
@@ -85,7 +92,7 @@ class DwdsVmClient {
     client.registerServiceCallback(
         'hotRestart',
         (request) => captureElapsedTime(
-            () => _hotRestart(chromeProxyService, client),
+            () => dwdsVmClient.hotRestart(chromeProxyService, client),
             (_) => DwdsEvent.hotRestart()));
     await client.registerService('hotRestart', 'DWDS');
 
@@ -138,7 +145,12 @@ class DwdsVmClient {
     });
     await client.registerService('_yieldControlToDDS', 'DWDS');
 
-    return DwdsVmClient(client, requestController, responseController);
+    return dwdsVmClient;
+  }
+
+  Future<Map<String, dynamic>> hotRestart(
+      ChromeProxyService chromeProxyService, VmService client) async {
+    return _hotRestartQueue.run(() => _hotRestart(chromeProxyService, client));
   }
 }