dart-lang/sdk
1
0
Fork 0
Code Issues Pull Requests Actions 2 Packages Projects Releases Wiki Activity

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

Browse Source 
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>
This commit is contained in:
Danny Tuppeny
2024-10-30 18:41:13 +00:00
committed by Commit Queue
parent 901f356c1f
commit 5b9d75ff20
10 changed files with 362 additions and 40 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/dds/CHANGELOG.md
+5
View File
@@ -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
+65 -12
View File
@@ -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
+95 -22
View File
@@ -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:
///
/// 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].
pkg/dds/lib/src/dap/protocol_converter.dart
+2 -2
View File
@@ -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
+1 -1
View File
@@ -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
+1 -1
View File
@@ -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.
pkg/dds/test/dap/integration/debug_eval_test.dart
+176
View File
@@ -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<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 {
pkg/dds/test/dap/integration/debug_logging_test.dart
-1
View File
@@ -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(
pkg/dds/test/dap/integration/test_client.dart
+11
View File
@@ -1170,6 +1170,17 @@ extension DapTestClientExtension on DapTestClient {
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
pkg/dds/test/dap/integration/test_support.dart
+6 -1
View File
@@ -306,12 +306,17 @@ environment:
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);
}