[dds/dap] Use ID Zones for evaluation requests and invalidate them on resume
Fixes https://github.com/dart-lang/sdk/issues/56794
Change-Id: Ied408503d22c4abb2c4f996f6c9b7847b8ad82be
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/389960
Reviewed-by: Derek Xu <derekx@google.com>
Commit-Queue: Ben Konyi <bkonyi@google.com>
Reviewed-by: Ben Konyi <bkonyi@google.com>
diff --git a/pkg/dds/CHANGELOG.md b/pkg/dds/CHANGELOG.md
index d5443de..c2ad2e3 100644
--- a/pkg/dds/CHANGELOG.md
+++ b/pkg/dds/CHANGELOG.md
@@ -3,6 +3,11 @@
- Updated the `devtools_shared` dependency to version `^11.0.0`.
- Made `runDartDevelopmentServiceFromCLI` pass the specified bind address
directly into `startDartDevelopmentService` without resolving the address.
+- [DAP] Evaluations now use Service ID Zones to more precisely control the
+ lifetime of instance references returned. This should avoid instances being
+ collected while execution is paused, while releasing them once execution
+ resumes.
+- Updated `vm_service` constraint to ^14.3.0.
# 4.2.7
- Added a new constant `RpcErrorCodes.kConnectionDisposed = -32010` for requests
diff --git a/pkg/dds/lib/src/dap/adapters/dart.dart b/pkg/dds/lib/src/dap/adapters/dart.dart
index 191f507..8c6b66a 100644
--- a/pkg/dds/lib/src/dap/adapters/dart.dart
+++ b/pkg/dds/lib/src/dap/adapters/dart.dart
@@ -956,6 +956,22 @@
sendResponse(_noResult);
break;
+ // Used by tests to force a GC for a given DAP threadId.
+ case '_collectAllGarbage':
+ final threadId = args?.args['threadId'] as int;
+ final isolateId = isolateManager.getThread(threadId)?.isolate.id;
+ // Trigger the GC.
+ if (isolateId != null) {
+ await vmService?.callMethod(
+ '_collectAllGarbage',
+ isolateId: isolateId,
+ );
+ }
+
+ // Respond to the incoming request.
+ sendResponse(_noResult);
+ break;
+
default:
await super.customRequest(request, args, sendResponse);
}
@@ -1088,11 +1104,10 @@
thread,
);
} else if (thread != null && frameIndex != null) {
- result = await vmService?.evaluateInFrame(
- thread.isolate.id!,
+ result = await vmEvaluateInFrame(
+ thread,
frameIndex,
expression,
- disableBreakpoints: true,
);
} else if (targetScriptFileUri != null &&
// Since we can't currently get a thread, we assume the first thread is
@@ -1105,12 +1120,7 @@
throw 'Unable to find the library for $targetScriptFileUri';
}
- result = await vmService?.evaluate(
- thread.isolate.id!,
- library.id!,
- expression,
- disableBreakpoints: true,
- );
+ result = await vmEvaluate(thread, library.id!, expression);
}
} catch (e) {
final rawMessage = '$e';
@@ -2384,11 +2394,54 @@
final expressionWithoutExceptionExpression =
expression.substring(threadExceptionExpression.length + 1);
- return vmService?.evaluate(
- thread.isolate.id!,
+ return vmEvaluate(
+ thread,
exception.id!,
expressionWithoutExceptionExpression,
- disableBreakpoints: true,
+ );
+ }
+
+ /// Sends a VM 'evaluate' request for [thread].
+ Future<vm.Response?> vmEvaluate(
+ ThreadInfo thread,
+ String targetId,
+ String expression, {
+ bool? disableBreakpoints = true,
+ }) async {
+ final isolateId = thread.isolate.id!;
+ final futureOrEvalZoneId = thread.currentEvaluationZoneId;
+ final evalZoneId = futureOrEvalZoneId is String
+ ? futureOrEvalZoneId
+ : await futureOrEvalZoneId;
+
+ return vmService?.evaluate(
+ isolateId,
+ targetId,
+ expression,
+ disableBreakpoints: disableBreakpoints,
+ idZoneId: evalZoneId,
+ );
+ }
+
+ /// Sends a VM 'evaluateInFrame' request for [thread].
+ Future<vm.Response?> vmEvaluateInFrame(
+ ThreadInfo thread,
+ int frameIndex,
+ String expression, {
+ bool? disableBreakpoints = true,
+ }) async {
+ final isolateId = thread.isolate.id!;
+ final futureOrEvalZoneId = thread.currentEvaluationZoneId;
+ final evalZoneId = futureOrEvalZoneId is String
+ ? futureOrEvalZoneId
+ : await futureOrEvalZoneId;
+
+ return vmService?.evaluateInFrame(
+ isolateId,
+ frameIndex,
+ expression,
+ disableBreakpoints: disableBreakpoints,
+ idZoneId: evalZoneId,
);
}
diff --git a/pkg/dds/lib/src/dap/isolate_manager.dart b/pkg/dds/lib/src/dap/isolate_manager.dart
index 8f77086..28989f6 100644
--- a/pkg/dds/lib/src/dap/isolate_manager.dart
+++ b/pkg/dds/lib/src/dap/isolate_manager.dart
@@ -356,7 +356,7 @@
// Finally, when we're resuming, all stored objects become invalid and
// we can drop them to save memory.
- thread.clearStoredData();
+ await thread.clearTemporaryData();
thread.hasPendingUserResume = true;
try {
@@ -400,13 +400,14 @@
}
}
- // When we're resuming, all stored objects become invalid and we can drop
- // to save memory.
- thread.clearStoredData();
-
+ final isolateId = thread.isolate.id!;
try {
- thread.hasPendingDapResume = true;
- await _adapter.vmService?.readyToResume(thread.isolate.id!);
+ // When we're resuming, all stored objects become invalid and we can drop
+ // to save memory.
+ await thread.clearTemporaryData();
+
+ // Finally, signal that we're ready to resume.
+ await _adapter.vmService?.readyToResume(isolateId);
} on UnimplementedError {
// Fallback to a regular resume if the DDS version doesn't support
// `readyToResume`:
@@ -426,8 +427,6 @@
} else {
rethrow;
}
- } finally {
- thread.hasPendingDapResume = false;
}
}
@@ -570,12 +569,7 @@
String type,
) async {
try {
- final result = await _adapter.vmService?.evaluateInFrame(
- thread.isolate.id!,
- 0,
- expression,
- disableBreakpoints: true,
- );
+ final result = await _adapter.vmEvaluateInFrame(thread, 0, expression);
if (result is vm.InstanceRef) {
return result;
@@ -1118,6 +1112,32 @@
var atAsyncSuspension = false;
int? exceptionReference;
+ /// A [Completer] that completes with the evaluation zone ID for this thread.
+ ///
+ /// The completer is created when the request to create an evaluation zone is
+ /// started (which is lazy, the first time evaluation is performed).
+ ///
+ /// When the Debug Adapter is ready to resume this Isolate, it will first
+ /// invalidate all evaluation IDs in this zone so that they can be collected.
+ /// If the [Completer] is null, no evaluation has occurred and invalidation
+ /// can be skipped.
+ Completer<String?>? _currentEvaluationZoneIdCompleter;
+
+ /// Returns the current evaluation zone ID.
+ ///
+ /// To avoid additional 'await's, may return a String? directly if the value
+ /// is already available.
+ FutureOr<String?> get currentEvaluationZoneId {
+ // We already have the value, avoid the Future.
+ if (_currentEvaluationZoneId != null) {
+ return _currentEvaluationZoneId;
+ }
+ return _createOrGetEvaluationZoneId();
+ }
+
+ /// The current evaluation zone ID (if available).
+ String? _currentEvaluationZoneId;
+
/// Whether this thread is currently known to be paused in the VM.
///
/// Because requests are async, this is not guaranteed to be always correct
@@ -1165,10 +1185,6 @@
/// has not yet been responded to.
var hasPendingUserResume = false;
- /// Whether this isolate has an in-flight DAP (readyToResume) resume request
- /// that has not yet been responded to.
- var hasPendingDapResume = false;
-
ThreadInfo(this._manager, this.threadId, this.isolate);
Future<T> getObject<T extends vm.Response>(vm.ObjRef ref) =>
@@ -1188,6 +1204,42 @@
return _manager.getScripts(isolate);
}
+ /// Returns the evaluation zone ID for this thread.
+ ///
+ /// If it has not been created yet, creates it. If creation is in progress,
+ /// returns the existing future.
+ Future<String?> _createOrGetEvaluationZoneId() async {
+ // If we already have a completer, the request is already in flight (or
+ // has completed).
+ var completer = _currentEvaluationZoneIdCompleter;
+ if (completer != null) {
+ return completer.future;
+ }
+
+ // Otherwise, we need to start the request.
+ _currentEvaluationZoneIdCompleter = completer = Completer();
+
+ try {
+ final response = await _manager._adapter.vmService?.createIdZone(
+ isolate.id!,
+ vm.IdZoneBackingBufferKind.kRing,
+ vm.IdAssignmentPolicy.kAlwaysAllocate,
+ // Default capacity is 512. Since these are short-lived (only while
+ // paused) and we don't want to prevent expanding Lists, use something a
+ // little bigger.
+ capacity: 2048,
+ );
+ _currentEvaluationZoneId = response?.id;
+ } catch (_) {
+ // If this request fails for any reason (perhaps the target VM does not
+ // support this request), we should just use `null` as the zone ID and not
+ // prevent any evaluation requests.
+ _currentEvaluationZoneId = null;
+ }
+ completer.complete(_currentEvaluationZoneId);
+ return _currentEvaluationZoneId;
+ }
+
/// Resolves a source file path (or URI) into a URI for the VM.
///
/// sdk-path/lib/core/print.dart -> dart:core/print.dart
@@ -1489,11 +1541,32 @@
pathSegments: fileLikeUri.pathSegments.sublist(0, keepSegments));
}
- /// Clears all data stored for this thread.
+ /// Clears all temporary stored for this thread. This includes:
///
- /// References to stored data become invalid when the thread is resumed.
- void clearStoredData() {
+ /// - dropping any variablesReferences
+ /// - invalidating the evaluation ID zone
+ ///
+ /// This is generally called when requesting execution continues, since any
+ /// evaluated references are not expected to live past this point.
+ ///
+ /// https://microsoft.github.io/debug-adapter-protocol/overview#lifetime-of-objects-references
+ Future<void> clearTemporaryData() async {
+ // Clear variablesReferences.
_manager.clearStoredData(this);
+
+ // Invalidate all existing references in this evaluation zone.
+ // If the completer is null, no zone has ever been created (or started to
+ // be created), so this can be skipped.
+ if (_currentEvaluationZoneIdCompleter != null) {
+ final futureOrEvalZoneId = currentEvaluationZoneId;
+ final evalZoneId = futureOrEvalZoneId is String
+ ? futureOrEvalZoneId
+ : await futureOrEvalZoneId;
+ if (evalZoneId != null) {
+ await _manager._adapter.vmService
+ ?.invalidateIdZone(isolate.id!, evalZoneId);
+ }
+ }
}
/// Attempts to get a [vm.LibraryRef] for the given [scriptFileUri].
diff --git a/pkg/dds/lib/src/dap/protocol_converter.dart b/pkg/dds/lib/src/dap/protocol_converter.dart
index 120f307..53cfc25 100644
--- a/pkg/dds/lib/src/dap/protocol_converter.dart
+++ b/pkg/dds/lib/src/dap/protocol_converter.dart
@@ -335,8 +335,8 @@
required bool allowCallingToString,
required VariableFormat? format,
}) async {
- final response = await service.evaluate(
- thread.isolate.id!,
+ final response = await _adapter.vmEvaluate(
+ thread,
instance.id!,
getterName,
);
diff --git a/pkg/dds/pubspec.yaml b/pkg/dds/pubspec.yaml
index e1fb7b4..da45485 100644
--- a/pkg/dds/pubspec.yaml
+++ b/pkg/dds/pubspec.yaml
@@ -29,7 +29,7 @@
sse: ^4.0.0
stack_trace: ^1.10.0
stream_channel: ^2.0.0
- vm_service: ^14.0.0
+ vm_service: ^14.3.0
web_socket_channel: '>=2.0.0 <4.0.0'
# We use 'any' version constraints here as we get our package versions from
diff --git a/pkg/dds/test/dap/integration/debug_breakpoints_test.dart b/pkg/dds/test/dap/integration/debug_breakpoints_test.dart
index cbd8e7f..f5e578c 100644
--- a/pkg/dds/test/dap/integration/debug_breakpoints_test.dart
+++ b/pkg/dds/test/dap/integration/debug_breakpoints_test.dart
@@ -606,7 +606,7 @@
final thread1 = stop1.threadId!;
// Attach a second debug adapter to it.
- final dap2 = await DapTestSession.setUp();
+ final dap2 = await DapTestSession.setUp(logPrefix: '(CLIENT2) ');
final client2 = dap2.client;
await Future.wait([
// We'll still get event for existing pause.
diff --git a/pkg/dds/test/dap/integration/debug_eval_test.dart b/pkg/dds/test/dap/integration/debug_eval_test.dart
index 6328880..6cd9dc6 100644
--- a/pkg/dds/test/dap/integration/debug_eval_test.dart
+++ b/pkg/dds/test/dap/integration/debug_eval_test.dart
@@ -284,6 +284,182 @@
);
});
+ test('variableReferences remain valid while an isolate is paused',
+ () async {
+ final client = dap.client;
+ final testFile =
+ dap.createTestFile(simpleBreakpointProgramWith50ExtraLines);
+ final breakpointLine = lineWith(testFile, breakpointMarker);
+ final stop = await client.hitBreakpoint(testFile, breakpointLine);
+
+ // We're paused at a breakpoint. Our evaluate results should continue to
+ // work even after GCs.
+ final threadId = stop.threadId!;
+ final topFrameId = await client.getTopFrameId(threadId);
+
+ // Evaluate something and get back a variablesReference.
+ final result = await client.expectEvalResult(
+ topFrameId,
+ 'DateTime(2000, 1, 1)',
+ 'DateTime',
+ );
+
+ // Ensure it remains valid even after GCs. This ensures both DAP preserves
+ // the variablesReference, and also that the VM preserved the instance
+ // reference.
+ for (var i = 0; i < 5; i++) {
+ await client.expectVariables(
+ result.variablesReference,
+ 'isUtc: false, eval: DateTime(2000, 1, 1).isUtc',
+ );
+ // Force GC.
+ await client.forceGc(threadId);
+ }
+ });
+
+ test('variableReferences become invalid after a resume', () async {
+ final client = dap.client;
+ final testFile =
+ dap.createTestFile(simpleBreakpointProgramWith50ExtraLines);
+ final breakpointLine = lineWith(testFile, breakpointMarker);
+ final stop = await client.hitBreakpoint(
+ testFile,
+ breakpointLine,
+ additionalBreakpoints: [breakpointLine + 1],
+ );
+
+ // We're paused at a breakpoint. Our evaluate results should only continue
+ // to work until we resume.
+ final threadId = stop.threadId!;
+ final topFrameId = await client.getTopFrameId(threadId);
+
+ // Evaluate something and get back a variablesReference.
+ final evalResult = await client.expectEvalResult(
+ topFrameId,
+ 'DateTime(2000, 1, 1)',
+ 'DateTime',
+ );
+
+ // Resume, which should invalidate the variablesReference.
+ await client.continue_(threadId);
+
+ // Verify the reference is no longer valid. This is because we clear
+ // the threads variable data on resume, and not because of VM Service
+ // Zone IDs. We have no way to validate the VM behaviour because we
+ // don't have the reference (it is abstracted from the DAP client) to
+ // verify.
+ expectResponseError(
+ client.variables(evalResult.variablesReference),
+ equals('Bad state: variablesReference is no longer valid'),
+ );
+ });
+
+ test('variableReferences become invalid after a step', () async {
+ final client = dap.client;
+ final testFile =
+ dap.createTestFile(simpleBreakpointProgramWith50ExtraLines);
+ final breakpointLine = lineWith(testFile, breakpointMarker);
+ final stop = await client.hitBreakpoint(testFile, breakpointLine);
+
+ // We're paused at a breakpoint. Our evaluate results should only continue
+ // to work until we resume.
+ final threadId = stop.threadId!;
+ final topFrameId = await client.getTopFrameId(threadId);
+
+ // Evaluate something and get back a variablesReference.
+ final evalResult = await client.expectEvalResult(
+ topFrameId,
+ 'DateTime(2000, 1, 1)',
+ 'DateTime',
+ );
+
+ // Step, which should also invalidate because we're basically unpausing
+ // and then pausing again.
+ await client.next(threadId);
+
+ // Verify the reference is no longer valid. This is because we clear
+ // the threads variable data on resume/step, and not because of VM Service
+ // Zone IDs. We have no way to validate the VM behaviour because we
+ // don't have the reference (it is abstracted from the DAP client) to
+ // verify.
+ expectResponseError(
+ client.variables(evalResult.variablesReference),
+ equals('Bad state: variablesReference is no longer valid'),
+ );
+ });
+
+ test('evaluation service zones are invalidated on resume', () async {
+ final client = dap.client;
+ final testFile = dap.createTestFile(simpleBreakpointProgram);
+ final breakpointLine = lineWith(testFile, breakpointMarker);
+ final stop = await client.hitBreakpoint(testFile, breakpointLine);
+
+ // Enable logging and capture any requests to the VM that are calling
+ // invalidateIdZone.
+ final loggedEvaluateInFrameRequests = client
+ .events('dart.log')
+ .map((event) => event.body as Map<String, Object?>)
+ .map((body) => body['message'] as String)
+ .where((message) => message.contains('"method":"invalidateIdZone"'))
+ .toList();
+ await client.custom('updateSendLogsToClient', {'enabled': true});
+
+ // Trigger an evaluation because evaluation zones are created lazily
+ // and we won't invalidate anything if we haven't created one.
+ await client.evaluate('0', frameId: stop.threadId!);
+
+ // Resume and wait for the app to terminate.
+ await Future.wait([
+ client.continue_(stop.threadId!),
+ client.event('terminated'),
+ ]);
+
+ // Verify that invalidate had been called.
+ expect(
+ await loggedEvaluateInFrameRequests,
+ isNotEmpty,
+ );
+ });
+
+ test('evaluation service zones are invalidated on step', () async {
+ final client = dap.client;
+ final testFile = dap.createTestFile(simpleBreakpointProgram);
+ final breakpointLine = lineWith(testFile, breakpointMarker);
+ final stop = await client.hitBreakpoint(testFile, breakpointLine);
+
+ // Enable logging and capture any requests to the VM that are calling
+ // invalidateIdZone.
+ final loggedEvaluateInFrameRequests = client
+ .events('dart.log')
+ .map((event) => event.body as Map<String, Object?>)
+ .map((body) => body['message'] as String)
+ .where((message) => message.contains('"method":"invalidateIdZone"'))
+ .toList();
+ await client.custom('updateSendLogsToClient', {'enabled': true});
+
+ // Trigger an evaluation because evaluation zones are created lazily
+ // and we won't invalidate anything if we haven't created one.
+ await client.evaluate('0', frameId: stop.threadId!);
+
+ // Step, which should also invalidate because we're basically unpausing
+ // and then pausing again.
+ await client.next(stop.threadId!);
+
+ // Then disable logging (so we don't get false positives from shutdown)
+ // and terminate.
+ await client.custom('updateSendLogsToClient', {'enabled': false});
+ await Future.wait([
+ client.terminate(),
+ client.event('terminated'),
+ ]);
+
+ // Verify that invalidate had been called.
+ expect(
+ await loggedEvaluateInFrameRequests,
+ isNotEmpty,
+ );
+ });
+
group('global evaluation', () {
test('can evaluate in a bin/ file when not paused given a bin/ URI',
() async {
diff --git a/pkg/dds/test/dap/integration/debug_logging_test.dart b/pkg/dds/test/dap/integration/debug_logging_test.dart
index 5e20f23..7643689 100644
--- a/pkg/dds/test/dap/integration/debug_logging_test.dart
+++ b/pkg/dds/test/dap/integration/debug_logging_test.dart
@@ -75,7 +75,6 @@
client.terminate(),
client.event('terminated'),
]);
- print(await loggedEvaluateInFrameRequests);
// Verify that the middle evaluation was logged but the others were not.
expect(
diff --git a/pkg/dds/test/dap/integration/test_client.dart b/pkg/dds/test/dap/integration/test_client.dart
index 9e743b6..59a540b 100644
--- a/pkg/dds/test/dap/integration/test_client.dart
+++ b/pkg/dds/test/dap/integration/test_client.dart
@@ -1170,6 +1170,17 @@
response.body as Map<String, Object?>);
}
+ /// A helper that calls the private `_collectAllGarbage` custom request to
+ /// force a GC.
+ Future<void> forceGc(int threadId) async {
+ await sendRequest(
+ {
+ 'threadId': threadId,
+ },
+ overrideCommand: '_collectAllGarbage',
+ );
+ }
+
/// A helper that verifies the variables list matches [expectedVariables].
///
/// [expectedVariables] is a simple text format of `name: value` for each
diff --git a/pkg/dds/test/dap/integration/test_support.dart b/pkg/dds/test/dap/integration/test_support.dart
index 0634566..4b34242 100644
--- a/pkg/dds/test/dap/integration/test_support.dart
+++ b/pkg/dds/test/dap/integration/test_support.dart
@@ -306,12 +306,17 @@
static Future<DapTestSession> setUp({
List<String>? additionalArgs,
bool forceVerboseLogging = false,
+ String? logPrefix = '',
}) async {
+ final logger = forceVerboseLogging || verboseLogging
+ ? (s) => print('$logPrefix$s')
+ : null;
+
final server = await startServer(additionalArgs: additionalArgs);
final client = await DapTestClient.connect(
server,
captureVmServiceTraffic: forceVerboseLogging || verboseLogging,
- logger: forceVerboseLogging || verboseLogging ? print : null,
+ logger: logger,
);
return DapTestSession._(server, client);
}
