diff --git a/pkg/dds/CHANGELOG.md b/pkg/dds/CHANGELOG.md index d5443de9d30..c2ad2e38ec4 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 191f50723c1..8c6b66a3b38 100644 --- a/pkg/dds/lib/src/dap/adapters/dart.dart +++ b/pkg/dds/lib/src/dap/adapters/dart.dart @@ -956,6 +956,22 @@ abstract class DartDebugAdapter 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 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 8f770862869..28989f646dc 100644 --- a/pkg/dds/lib/src/dap/isolate_manager.dart +++ b/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? _currentEvaluationZoneIdCompleter; + + /// Returns the current evaluation zone ID. + /// + /// To avoid additional 'await's, may return a String? directly if the value + /// is already available. + FutureOr 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 getObject(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 _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: /// - /// 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 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 120f307e110..53cfc257268 100644 --- a/pkg/dds/lib/src/dap/protocol_converter.dart +++ b/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, ); diff --git a/pkg/dds/pubspec.yaml b/pkg/dds/pubspec.yaml index e1fb7b4dc5c..da45485c21f 100644 --- a/pkg/dds/pubspec.yaml +++ b/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 diff --git a/pkg/dds/test/dap/integration/debug_breakpoints_test.dart b/pkg/dds/test/dap/integration/debug_breakpoints_test.dart index cbd8e7fe872..f5e578c964a 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 @@ void main(List 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. diff --git a/pkg/dds/test/dap/integration/debug_eval_test.dart b/pkg/dds/test/dap/integration/debug_eval_test.dart index 63288807c17..6cd9dc676c3 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 @@ void foo() { ); }); + 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) + .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) + .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 5e20f239923..7643689eca9 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 @@ main() { 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 9e743b6d5c2..59a540b074e 100644 --- a/pkg/dds/test/dap/integration/test_client.dart +++ b/pkg/dds/test/dap/integration/test_client.dart @@ -1170,6 +1170,17 @@ extension DapTestClientExtension on DapTestClient { response.body as Map); } + /// A helper that calls the private `_collectAllGarbage` custom request to + /// force a GC. + Future 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 0634566a7df..4b34242ebc0 100644 --- a/pkg/dds/test/dap/integration/test_support.dart +++ b/pkg/dds/test/dap/integration/test_support.dart @@ -306,12 +306,17 @@ environment: static Future setUp({ List? 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); }