[dds/dap] Ensure breakpoint modifications do not drop resolution events

When adding new breakpoints, DAP clients send a full new set of breakpoints which results in us removing and re-adding all breakpoints (for a file).

When we re-add a breakpoint that already existed, we would sometimes get the BreakpointAdded event _before_ the addBreakpointWithScriptUri request completed. We couldn't process the event because without the original request completing we could not map the VM breakpoint back to the clients breakpoint to generate the event.

This could result in resolution events being lost, and breakpoints becoming unresolved when you modified them (depending on timing).

This change collects queues and replays any BreakpointAdded/BreakpointResolved events that arrived when the breakpoint ID is unknown, and when `addBreakpointWithScriptUri` completes, it immediately processes any items in this queue.

Fixes Dart-Code/Dart-Code#4599

Change-Id: I11daaf99b786ab94f1cc93f9fd38a4f1e241320f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/310620
Commit-Queue: Ben Konyi <[email protected]>
Reviewed-by: Ben Konyi <[email protected]>

Author: DanTup

pkg/dds/CHANGELOG.md

@@ -2,6 +2,7 @@
 - [DAP] `runInTerminal` requests are now sent after first responding to the `launchRequest`.
 - [DAP] Skipped tests are now marked with `!` instead of `✓` in `Output` events.
 - [DAP] Implemented `pause` request.
+- [DAP] Fixed an issue that could leave breakpoints unresolved when adding/removing other breakpoints in a file.
 - Fixed a bug that was preventing clients from receiving `IsolateReload` events
   (see https://dartbug.com/49491).
 

pkg/dds/lib/src/dap/isolate_manager.dart

@@ -71,8 +71,20 @@ class IsolateManager {
 
   /// Tracks client breakpoints by the ID assigned by the VM so we can look up
   /// conditions/logpoints when hitting breakpoints.
+  ///
+  /// When an item is added to this map, any pending events in
+  /// [_pendingBreakpointEvents] MUST be processed immediately.
   final Map<String, ClientBreakpoint> _clientBreakpointsByVmId = {};
 
+  /// Tracks breakpoints for which we have received `BreakpointAdded` or
+  /// `BreakpointResolved` events, but whose `addBreakpointWithScriptUri`
+  /// requests have not yet completed.
+  ///
+  /// We cannot process these events until after we get the breakpoint ID
+  /// back from the VM. This queue MUST be processed immediately any time a
+  /// breakpoint is added to [_clientBreakpointsByVmId].
+  final Map<String, PendingBreakpointActions> _pendingBreakpointEvents = {};
+
   /// Tracks breakpoints created in the VM so they can be removed when the
   /// editor sends new breakpoints (currently the editor just sends a new list
   /// and not requests to add/remove).
@@ -624,9 +636,24 @@ class IsolateManager {
     final existingBreakpoint = _clientBreakpointsByVmId[breakpointId];
     if (existingBreakpoint == null) {
       // If we can't match this breakpoint up, we cannot get its ID or send
-      // events for it. This can happen if a breakpoint is being resolved just
-      // as a user changes breakpoints, so we have replaced our collection with
-      // a new set before we processed this event.
+      // events for it. This can happen if:
+      //
+      // 1. A breakpoint is being resolved just as a user changes breakpoints,
+      //    so we have replaced our collection with a new set before we
+      //    processed this event.
+      // 2. We got a `BreakpointAdded`/`BreakpointUpdated` event prior to the
+      //    response to `addBreakpointWithScriptUri`. In this case, we need to
+      //    process this once that has completed and we can match up the IDs.
+      //
+      // It's important we don't miss any events for (2), so queue them up so
+      // they can be handled if/when the breakpoint is handled in a
+      // `setBreakpointWithScriptUri` response.
+      final queue = _pendingBreakpointEvents.putIfAbsent(
+        breakpointId,
+        PendingBreakpointActions.new,
+      );
+      // Queue another call back to this function later.
+      queue.queueAction(() => _handleBreakpointAddedOrResolved(event));
       return;
     }
 
@@ -791,8 +818,20 @@ class IsolateManager {
           final vmBp = await service.addBreakpointWithScriptUri(
               isolateId, vmUri.toString(), bp.breakpoint.line,
               column: bp.breakpoint.column);
-          existingBreakpointsForIsolateAndUri[vmBp.id!] = vmBp;
-          _clientBreakpointsByVmId[vmBp.id!] = bp;
+          final vmBpId = vmBp.id!;
+          existingBreakpointsForIsolateAndUri[vmBpId] = vmBp;
+          _clientBreakpointsByVmId[vmBpId] = bp;
+
+          // Now we have the ID of this breakpoint, append an action that
+          // processes any pending events.
+          final pendingEvents = _pendingBreakpointEvents[vmBpId];
+          if (pendingEvents != null) {
+            bp.queueAction(() {
+              pendingEvents.completer.complete();
+              return pendingEvents._lastActionFuture;
+            });
+            _pendingBreakpointEvents.remove(vmBpId);
+          }
         } catch (e) {
           // Swallow errors setting breakpoints rather than failing the whole
           // request as it's very easy for editors to send us breakpoints that
@@ -1254,6 +1293,30 @@ class ClientBreakpoint {
   }
 }
 
+/// Tracks actions resulting from `BreakpointAdded`/`BreakpointResolved` events
+/// that arrive before the `addBreakpointWithScriptUri` request completes.
+///
+/// These events need to be chained into the end of the [ClientBreakpoint] once
+/// that request completes.
+class PendingBreakpointActions {
+  /// A completer that will trigger processing of the queue.
+  final completer = Completer<void>();
+
+  /// A [Future] that completes with the last action in the queue.
+  late Future<void> _lastActionFuture;
+
+  PendingBreakpointActions() {
+    _lastActionFuture = completer.future;
+  }
+
+  /// Queues an action to run after all previous actions.
+  FutureOr<T> queueAction<T>(FutureOr<T> Function() action) {
+    final actionFuture = _lastActionFuture.then((_) => action());
+    _lastActionFuture = actionFuture;
+    return actionFuture;
+  }
+}
+
 class StoredData {
   final ThreadInfo thread;
   final Object data;

pkg/dds/test/dap/integration/debug_breakpoints_test.dart

@@ -64,6 +64,45 @@ main() {
       expect(updatedBreakpoint.line, expectedResolvedBreakpointLine);
     });
 
+    test('resolves modified breakpoints', () async {
+      final client = dap.client;
+      final testFile = dap.createTestFile(simpleMultiBreakpointProgram);
+      final breakpointLine = lineWith(testFile, breakpointMarker);
+
+      // Start the app and hit the initial breakpoint.
+      await client.hitBreakpoint(testFile, breakpointLine);
+
+      // Collect IDs of all breakpoints that get resolved.
+      final resolvedBreakpoints = <int>{};
+      final breakpointResolveSubscription =
+          client.breakpointChangeEvents.listen((event) {
+        if (event.breakpoint.verified) {
+          resolvedBreakpoints.add(event.breakpoint.id!);
+        } else {
+          resolvedBreakpoints.remove(event.breakpoint.id!);
+        }
+      });
+
+      // Add breakpoints to the 4 lines after the current one, one at a time.
+      // Capture the IDs of all breakpoints added.
+      final breakpointLinesToSend = <int>[breakpointLine];
+      final addedBreakpoints = <int>{};
+      for (var i = 1; i <= 4; i++) {
+        breakpointLinesToSend.add(breakpointLine + i);
+        final response =
+            await client.setBreakpoints(testFile, breakpointLinesToSend);
+        for (final breakpoint in response.breakpoints) {
+          addedBreakpoints.add(breakpoint.id!);
+        }
+      }
+
+      await pumpEventQueue(times: 5000);
+      await breakpointResolveSubscription.cancel();
+
+      // Ensure every breakpoint that was added was also resolved.
+      expect(resolvedBreakpoints, addedBreakpoints);
+    });
+
     test('responds to setBreakpoints before any breakpoint events', () async {
       final client = dap.client;
       final testFile = dap.createTestFile(simpleBreakpointResolutionProgram);

pkg/dds/test/dap/integration/test_client.dart

@@ -668,13 +668,18 @@ extension DapTestClientExtension on DapTestClient {
   }
 
   /// Sets breakpoints at [lines] in [file].
-  Future<void> setBreakpoints(File file, List<int> lines) async {
-    await sendRequest(
+  Future<SetBreakpointsResponseBody> setBreakpoints(
+      File file, List<int> lines) async {
+    final response = await sendRequest(
       SetBreakpointsArguments(
         source: Source(path: _normalizeBreakpointPath(file.path)),
         breakpoints: lines.map((line) => SourceBreakpoint(line: line)).toList(),
       ),
     );
+
+    return SetBreakpointsResponseBody.fromJson(
+      response.body as Map<String, Object?>,
+    );
   }
 
   /// Normalizes a non-breakpoint path being sent to the debug adapter based on

pkg/dds/test/dap/integration/test_scripts.dart

@@ -140,6 +140,20 @@ const simpleBreakpointProgram = '''
   }
 ''';
 
+/// A simple Dart script that prints the numbers from 1 to 5.
+///
+/// A breakpoint marker is on the line that prints '1' and the subsequent 4
+/// lines are valid targets for breakpoints.
+const simpleMultiBreakpointProgram = '''
+  void main(List<String> args) async {
+    print('1'); $breakpointMarker
+    print('2');
+    print('3');
+    print('4');
+    print('5');
+  }
+''';
+
 /// A Dart script that uses [Isolate.run] to run a short-lived isolate and has
 /// a `debugger()` call after the isolate completes to ensure the app does not
 /// immediately exit.