[dds/dap] Fix breakpoint resolution races when there are multiple isolates
When there are multiple isolates, we may get breakpoint responses/events like this:
- Request/Response to add breakpoint to Isolate 1
- Request/Response to add breakpoint to Isolate 2
- BreakpointAdded for Isolate 1
- BreakpointResolved for Isolate 1
- BreakpointAdded for Isolate 2
Because Isolate 2 did not load the script, the last breakpoint was never resolved. However because the breakpoint ID matched, we would forward this event to the client and un-resolve the previously resolved breakpoint. This would result in odd behaviour in VS Code.
Additionally, the VM may return the same BM breakpoint ID for what the client thinks are two breakpoints (they are on different lines, but resolve to the same location) so when we get a resolved breakpoint, we need to handle both:
- Client breakpoints that have already been transmitted
- Future client breakpoints that may resolve to this same VM breakpoint
The previous code assumed that a BreakpointResolved event could be handled just once. Either for an existing breakpoint, or a future one.
This change swaps from storing queued events to storing the resolution information for each breakpoint, and it does this even if there was an existing breakpoint (in case the breakpoint is reused in future).
Fixes at least some of https://github.com/Dart-Code/Dart-Code/issues/4598
Change-Id: I53b92debfaa0c8f538dc8d67966854bb89634708
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/311480
Reviewed-by: Ben Konyi <bkonyi@google.com>
Commit-Queue: Ben Konyi <bkonyi@google.com>
diff --git a/pkg/dds/CHANGELOG.md b/pkg/dds/CHANGELOG.md
index 0d81d77..f129c9f 100644
--- a/pkg/dds/CHANGELOG.md
+++ b/pkg/dds/CHANGELOG.md
@@ -1,3 +1,6 @@
+# 2.9.2
+- [DAP] Fixed an issue that could cause breakpoints to become unresolved when there are multiple isolates (such as during a test run).
+
# 2.9.1
- [DAP] A new configuration option `bool? showGettersInDebugViews` allows getters to be shown wrapped in Variables/Evaluation responses so that they can be lazily expanded by the user. `evaluateGettersInDebugViews` must be `false` for this behaviour.
- [DAP] `runInTerminal` requests are now sent after first responding to the `launchRequest`.
diff --git a/pkg/dds/lib/src/dap/isolate_manager.dart b/pkg/dds/lib/src/dap/isolate_manager.dart
index 4011ac9..49251cb 100644
--- a/pkg/dds/lib/src/dap/isolate_manager.dart
+++ b/pkg/dds/lib/src/dap/isolate_manager.dart
@@ -72,18 +72,27 @@
/// Tracks client breakpoints by the ID assigned by the VM so we can look up
/// conditions/logpoints when hitting breakpoints.
///
+ /// Because the VM might return the same breakpoint for multiple
+ /// `addBreakpointWithScriptUri` calls (if they immediately resolve to the
+ /// same location) there may be multiple client breakpoints for a given VM
+ /// breakpoint ID.
+ ///
/// When an item is added to this map, any pending events in
/// [_pendingBreakpointEvents] MUST be processed immediately.
- final Map<String, ClientBreakpoint> _clientBreakpointsByVmId = {};
+ final Map<String, List<ClientBreakpoint>> _clientBreakpointsByVmId = {};
- /// Tracks breakpoints for which we have received `BreakpointAdded` or
- /// `BreakpointResolved` events, but whose `addBreakpointWithScriptUri`
- /// requests have not yet completed.
+ /// Tracks `BreakpointAdded` or `BreakpointResolved` events for VM
+ /// breakpoints.
///
- /// 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 = {};
+ /// These are kept for all breakpoints until they are removed by the VM
+ /// because it's always possible that the VM will reuse a breakpoint ID (eg.
+ /// if we add a new breakpoint that resolves to the same location as another
+ /// breakpoint).
+ ///
+ /// When new breakpoints are added by the client, we must check this map to
+ /// see it's al already-resolved breakpoint so that we can send resolution
+ /// info to the client.
+ final Map<String, vm.Event> _breakpointResolvedEventsByVmId = {};
/// 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
@@ -552,8 +561,13 @@
// Look up the client breakpoints that correspond to the VM breakpoint(s)
// we hit. It's possible some of these may be missing because we could
// hit a breakpoint that was set before we were attached.
+ //
+ // When multiple client breakpoints have been folded into a single VM
+ // breakpoint, we (arbitrarily) use the first one for conditions and
+ // logpoints.
final clientBreakpoints = event.pauseBreakpoints!
- .map((bp) => _clientBreakpointsByVmId[bp.id!]?.breakpoint)
+ .map((bp) =>
+ _clientBreakpointsByVmId[bp.id!]?.firstOrNull?.breakpoint)
.toSet();
// Split into logpoints (which just print messages) and breakpoints.
@@ -629,49 +643,57 @@
/// Handles 'BreakpointAdded'/'BreakpointResolved' events from the VM,
/// informing the client of updated information about the breakpoint.
+ ///
+ /// Information about unresolved breakpoints will be ignored to avoid
+ /// overwriting resolved breakpoint info with unresolved/stale info in the
+ /// case of multiple isolates where they haven't all loaded the scripts that
+ /// we added breakpoints for.
void _handleBreakpointAddedOrResolved(vm.Event event) {
final breakpoint = event.breakpoint!;
final breakpointId = breakpoint.id!;
- 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:
- //
- // 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));
+ if (!(breakpoint.resolved ?? false)) {
+ // Unresolved breakpoint, don't need to do anything.
return;
}
- // Location may be [SourceLocation] or [UnresolvedSourceLocaion] depending
- // on whether this is an Added or Resolved event.
+ // Store this event so if we get any future breakpoints that resolve to this
+ // VM breakpoint, we can access the resolution info.
+ _breakpointResolvedEventsByVmId[breakpointId] = event;
+
+ // And for existing breakpoints, send (or queue) resolved events.
+ final existingBreakpoints = _clientBreakpointsByVmId[breakpointId];
+ for (final existingBreakpoint in existingBreakpoints ?? const []) {
+ queueBreakpointResolutionEvent(event, existingBreakpoint);
+ }
+ }
+
+ /// Queues a breakpoint resolution event that passes resolution info from
+ /// the VM back to the client.
+ ///
+ /// This queue will be processed only after the client has been given the ID
+ /// of this breakpoint. If that has already happened, the event will be
+ /// processed on the next task queue iteration.
+ void queueBreakpointResolutionEvent(
+ vm.Event addedOrResolvedEvent,
+ ClientBreakpoint clientBreakpoint,
+ ) {
+ assert(addedOrResolvedEvent.breakpoint != null);
+ final breakpoint = addedOrResolvedEvent.breakpoint!;
+ assert(breakpoint.resolved ?? false);
+
+ // This is always resolved because of the check above.
final location = breakpoint.location;
- final resolvedLocation = location is vm.SourceLocation ? location : null;
- final unresolvedLocation =
- location is vm.UnresolvedSourceLocation ? location : null;
+ final resolvedLocation = location as vm.SourceLocation;
final updatedBreakpoint = Breakpoint(
- id: existingBreakpoint.id,
- line: resolvedLocation?.line ?? unresolvedLocation?.line,
- column: resolvedLocation?.column ?? unresolvedLocation?.column,
- verified: breakpoint.resolved ?? false,
+ id: clientBreakpoint.id,
+ line: resolvedLocation.line,
+ column: resolvedLocation.column,
+ verified: true,
);
// Ensure we don't send the breakpoint event until the client has been
- // given the breakpoint ID.
- existingBreakpoint.queueAction(
+ // given the breakpoint ID by queueing it.
+ clientBreakpoint.queueAction(
() => _adapter.sendEvent(
BreakpointEventBody(breakpoint: updatedBreakpoint, reason: 'changed'),
),
@@ -820,17 +842,19 @@
column: bp.breakpoint.column);
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);
+ // Store this client breakpoint by the VM ID, so when we get events
+ // from the VM we can map them back to client breakpoints (for example
+ // to send resolved events).
+ _clientBreakpointsByVmId.putIfAbsent(vmBpId, () => []).add(bp);
+
+ // Queue any resolved events that may have already arrived
+ // (either because the VM sent them before responding to us, or
+ // because it gave us an existing VM breakpoint because it resolved to
+ // the same location as another).
+ final resolvedEvent = _breakpointResolvedEventsByVmId[vmBpId];
+ if (resolvedEvent != null) {
+ queueBreakpointResolutionEvent(resolvedEvent, bp);
}
} catch (e) {
// Swallow errors setting breakpoints rather than failing the whole
diff --git a/pkg/dds/pubspec.yaml b/pkg/dds/pubspec.yaml
index 31c3cf5..f5c9d56 100644
--- a/pkg/dds/pubspec.yaml
+++ b/pkg/dds/pubspec.yaml
@@ -1,5 +1,5 @@
name: dds
-version: 2.9.1
+version: 2.9.2
description: >-
A library used to spawn the Dart Developer Service, used to communicate with
a Dart VM Service instance.
diff --git a/pkg/dds/test/dap/integration/dart_test_test.dart b/pkg/dds/test/dap/integration/dart_test_test.dart
index da86102..d64b6b2 100644
--- a/pkg/dds/test/dap/integration/dart_test_test.dart
+++ b/pkg/dds/test/dap/integration/dart_test_test.dart
@@ -2,6 +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:dap/dap.dart';
import 'package:test/test.dart';
import 'test_client.dart';
@@ -144,6 +145,118 @@
], eagerError: true);
});
+ test('resolves and updates breakpoints', () async {
+ final client = dap.client;
+ final testFile =
+ dap.createTestFile(simpleTestBreakpointResolutionProgram);
+ final setBreakpointLine = lineWith(testFile, breakpointMarker);
+ final expectedResolvedBreakpointLine = setBreakpointLine + 1;
+
+ // Collect any breakpoint changes during the run.
+ final breakpointChangesFuture = client.breakpointChangeEvents.toList();
+
+ Future<SetBreakpointsResponseBody> setBreakpointFuture;
+ await Future.wait([
+ client
+ .expectStop('breakpoint',
+ file: testFile, line: expectedResolvedBreakpointLine)
+ .then((_) => client.terminate()),
+ client.initialize(),
+ setBreakpointFuture = client.setBreakpoint(testFile, setBreakpointLine),
+ client.launch(testFile.path),
+ ], eagerError: true);
+
+ // The initial setBreakpointResponse should always return unverified
+ // because we verify using the BreakpointAdded/BreakpointResolved events.
+ final setBreakpointResponse = await setBreakpointFuture;
+ expect(setBreakpointResponse.breakpoints, hasLength(1));
+ final setBreakpoint = setBreakpointResponse.breakpoints.single;
+ expect(setBreakpoint.verified, isFalse);
+
+ // The last breakpoint change we had should be verified and also update
+ // the line to [expectedResolvedBreakpointLine] since the breakpoint was
+ // on a blank line.
+ final breakpointChanges = await breakpointChangesFuture;
+ final updatedBreakpoint = breakpointChanges.last.breakpoint;
+ expect(updatedBreakpoint.verified, isTrue);
+ expect(updatedBreakpoint.line, expectedResolvedBreakpointLine);
+ });
+
+ test('resolves modified breakpoints', () async {
+ final client = dap.client;
+ final testFile = dap.createTestFile(simpleTestMultiBreakpointProgram);
+ 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(simpleTestBreakpointResolutionProgram);
+ final setBreakpointLine = lineWith(testFile, breakpointMarker);
+
+ // Run the app and get to a breakpoint. This will allow us to add new
+ // breakpoints in the same file that are _immediately_ resolved.
+ await Future.wait([
+ client.initialize(),
+ client.expectStop('breakpoint'),
+ client.setBreakpoint(testFile, setBreakpointLine),
+ client.launch(testFile.path),
+ ], eagerError: true);
+
+ // Call setBreakpoint again, and ensure it response before we get any
+ // breakpoint change events because we require their IDs before the change
+ // events are sent.
+ var setBreakpointsResponded = false;
+ await Future.wait([
+ client.breakpointChangeEvents.first.then((_) {
+ if (!setBreakpointsResponded) {
+ throw 'breakpoint change event arrived before '
+ 'setBreakpoints completed';
+ }
+ }),
+ client
+ // Send 50 breakpoints for lines 1-50 to ensure we spend some time
+ // sending requests to the VM to allow events to start coming back
+ // from the VM before we complete. Without this, the test can pass
+ // even without the fix.
+ .setBreakpoints(testFile, List.generate(50, (index) => index))
+ .then((_) => setBreakpointsResponded = true),
+ ]);
+ });
+
test('rejects attaching', () async {
final client = dap.client;
diff --git a/pkg/dds/test/dap/integration/test_scripts.dart b/pkg/dds/test/dap/integration/test_scripts.dart
index eea298f..a72b9b1 100644
--- a/pkg/dds/test/dap/integration/test_scripts.dart
+++ b/pkg/dds/test/dap/integration/test_scripts.dart
@@ -238,6 +238,43 @@
}
''';
+/// A simple test that should pass and contains a comment marker
+/// '// BREAKPOINT' on a blank line where a breakpoint should be resolved
+/// to the next line.
+const simpleTestBreakpointResolutionProgram = '''
+ import 'package:test/test.dart';
+
+ void main() {
+ group('group 1', () {
+ test('passing test', () {
+ $breakpointMarker
+ expect(1, equals(1));
+ });
+ });
+ }
+''';
+
+/// A simple test 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 simpleTestMultiBreakpointProgram = '''
+ import 'package:test/test.dart';
+
+ void main() {
+ group('group 1', () {
+ test('passing test', () {
+ print('1'); $breakpointMarker
+ print('2');
+ print('3');
+ print('4');
+ print('5');
+ expect(1, equals(1));
+ });
+ });
+ }
+''';
+
/// Matches for the expected output of [simpleTestProgram].
final simpleTestProgramExpectedOutput = [
// First test
