[dds/dap] Use ID Zones for evaluation requests and invalidate them on resume

Fixes #56794

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

Author: DanTup

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

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

@@ -956,6 +956,22 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
         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 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
           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 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
           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 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
     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,
     );
   }
 

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

@@ -356,7 +356,7 @@ class IsolateManager {
 
     // 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 @@ class IsolateManager {
       }
     }
 
-    // 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 @@ class IsolateManager {
       } else {
         rethrow;
       }
-    } finally {
-      thread.hasPendingDapResume = false;
     }
   }
 
@@ -570,12 +569,7 @@ class IsolateManager {
     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 @@ class ThreadInfo with FileUtils {
   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 @@ class ThreadInfo with FileUtils {
   /// 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 @@ class ThreadInfo with FileUtils {
     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 @@ class ThreadInfo with FileUtils {
         pathSegments: fileLikeUri.pathSegments.sublist(0, keepSegments));
   }
 
-  /// Clears all data stored for this thread.
+  /// Clears all temporary stored for this thread. This includes:
+  ///
+  /// - 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.
   ///
-  /// References to stored data become invalid when the thread is resumed.
-  void clearStoredData() {
+  /// 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].

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

@@ -335,8 +335,8 @@ class ProtocolConverter {
     required bool allowCallingToString,
     required VariableFormat? format,
   }) async {
-    final response = await service.evaluate(
-      thread.isolate.id!,
+    final response = await _adapter.vmEvaluate(
+      thread,
       instance.id!,
       getterName,
     );

pkg/dds/pubspec.yaml

@@ -29,7 +29,7 @@ dependencies:
   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

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

@@ -606,7 +606,7 @@ void main(List<String> args) async {
       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.