[dds] Improve suppression of auto-resuming threads when attaching to processes
Hopefully fixes https://github.com/dart-lang/sdk/issues/48274.
Change-Id: I893a1f5dee54410986644a52b7c2bb406d4e51f3
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/237683
Reviewed-by: Ben Konyi <bkonyi@google.com>
Commit-Queue: Ben Konyi <bkonyi@google.com>
diff --git a/pkg/dds/lib/src/dap/adapters/dart.dart b/pkg/dds/lib/src/dap/adapters/dart.dart
index 83c9d9d..ab9d049 100644
--- a/pkg/dds/lib/src/dap/adapters/dart.dart
+++ b/pkg/dds/lib/src/dap/adapters/dart.dart
@@ -482,6 +482,10 @@
isAttach = true;
_subscribeToOutputStreams = true;
+ // When attaching to a process, suppress auto-resuming isolates until the
+ // first time the user resumes anything.
+ _isolateManager.autoResumeStartingIsolates = false;
+
// Common setup.
await _prepareForLaunchOrAttach(null);
@@ -538,13 +542,11 @@
/// The URI protocol will be changed to ws/wss but otherwise not normalised.
/// The caller should handle any other normalisation (such as adding /ws to
/// the end if required).
- ///
- /// If [resumeIfStarting] is true, isolates waiting to start will
- /// automatically be resumed. This is usually desired in launch requests, but
- /// not when attaching.
Future<void> connectDebugger(
Uri uri, {
- required bool resumeIfStarting,
+ // TODO(dantup): Remove this after parameter after updating the Flutter
+ // DAP to not pass it.
+ bool? resumeIfStarting,
}) async {
// Start up a DDS instance for this VM.
if (enableDds) {
@@ -622,7 +624,7 @@
await debuggerConnected(vmInfo);
await _withErrorHandling(
- () => _configureExistingIsolates(vmService, vmInfo, resumeIfStarting),
+ () => _configureExistingIsolates(vmService, vmInfo),
);
_debuggerInitializedCompleter.complete();
@@ -633,7 +635,6 @@
Future<void> _configureExistingIsolates(
vm.VmService vmService,
vm.VM vmInfo,
- bool resumeIfStarting,
) async {
final existingIsolateRefs = vmInfo.isolates;
final existingIsolates = existingIsolateRefs != null
@@ -659,12 +660,11 @@
if (isolate.pauseEvent?.kind?.startsWith('Pause') ?? false) {
await _isolateManager.handleEvent(
isolate.pauseEvent!,
- resumeIfStarting: resumeIfStarting,
);
} else if (isolate.runnable == true) {
// If requested, automatically resume. Otherwise send a Stopped event to
// inform the client UI the thread is paused.
- if (resumeIfStarting) {
+ if (_isolateManager.autoResumeStartingIsolates) {
await _isolateManager.resumeIsolate(isolate);
} else {
_isolateManager.sendStoppedOnEntryEvent(thread.threadId);
diff --git a/pkg/dds/lib/src/dap/adapters/dart_cli_adapter.dart b/pkg/dds/lib/src/dap/adapters/dart_cli_adapter.dart
index 06def69..1c8be40 100644
--- a/pkg/dds/lib/src/dap/adapters/dart_cli_adapter.dart
+++ b/pkg/dds/lib/src/dap/adapters/dart_cli_adapter.dart
@@ -94,7 +94,7 @@
if (debug) {
vmServiceInfoFile = generateVmServiceInfoFile();
unawaited(waitForVmServiceInfoFile(logger, vmServiceInfoFile)
- .then((uri) => connectDebugger(uri, resumeIfStarting: true)));
+ .then((uri) => connectDebugger(uri)));
}
final vmArgs = <String>[
@@ -191,7 +191,7 @@
? Uri.parse(vmServiceUri)
: await waitForVmServiceInfoFile(logger, File(vmServiceInfoFile!));
- unawaited(connectDebugger(uri, resumeIfStarting: false));
+ unawaited(connectDebugger(uri));
}
/// Calls the client (via a `runInTerminal` request) to spawn the process so
diff --git a/pkg/dds/lib/src/dap/adapters/dart_test_adapter.dart b/pkg/dds/lib/src/dap/adapters/dart_test_adapter.dart
index 716fcfe..0a2b63c 100644
--- a/pkg/dds/lib/src/dap/adapters/dart_test_adapter.dart
+++ b/pkg/dds/lib/src/dap/adapters/dart_test_adapter.dart
@@ -79,7 +79,7 @@
if (debug) {
vmServiceInfoFile = generateVmServiceInfoFile();
unawaited(waitForVmServiceInfoFile(logger, vmServiceInfoFile)
- .then((uri) => connectDebugger(uri, resumeIfStarting: true)));
+ .then((uri) => connectDebugger(uri)));
}
final vmArgs = <String>[
diff --git a/pkg/dds/lib/src/dap/isolate_manager.dart b/pkg/dds/lib/src/dap/isolate_manager.dart
index d13e8fa..df7dd76 100644
--- a/pkg/dds/lib/src/dap/isolate_manager.dart
+++ b/pkg/dds/lib/src/dap/isolate_manager.dart
@@ -56,6 +56,16 @@
/// [debugExternalPackageLibraries] in one step.
bool debugExternalPackageLibraries = true;
+ /// Whether to automatically resume new isolates after configuring them.
+ ///
+ /// This setting is almost always `true` because isolates are paused only so
+ /// we can configure them (send breakpoints, pause-on-exceptions,
+ /// setLibraryDebuggables) without races. It is set to `false` during the
+ /// initial connection of an `attachRequest` to allow paused isolates to
+ /// remain paused. In this case, it will be automatically re-set to `true` the
+ /// first time the user resumes.
+ bool autoResumeStartingIsolates = true;
+
/// The root of the Dart SDK containing the VM running the debug adapter.
late final String sdkRoot;
@@ -138,13 +148,7 @@
ThreadInfo? getThread(int threadId) => _threadsByThreadId[threadId];
/// Handles Isolate and Debug events.
- ///
- /// If [resumeIfStarting] is `true`, PauseStart/PausePostStart events will be
- /// automatically resumed from.
- Future<void> handleEvent(
- vm.Event event, {
- bool resumeIfStarting = true,
- }) async {
+ Future<void> handleEvent(vm.Event event) async {
final isolateId = event.isolate?.id!;
final eventKind = event.kind;
@@ -163,7 +167,7 @@
if (eventKind == vm.EventKind.kIsolateExit) {
_handleExit(event);
} else if (eventKind?.startsWith('Pause') ?? false) {
- await _handlePause(event, resumeIfStarting: resumeIfStarting);
+ await _handlePause(event);
} else if (eventKind == vm.EventKind.kResume) {
_handleResumed(event);
}
@@ -236,6 +240,11 @@
/// [vm.StepOption.kOver], a [StepOption.kOverAsyncSuspension] step will be
/// sent instead.
Future<void> resumeThread(int threadId, [String? resumeType]) async {
+ // The first time a user resumes a thread is our signal that the app is now
+ // "running" and future isolates can be auto-resumed. This only affects
+ // attach, as it's already `true` for launch requests.
+ autoResumeStartingIsolates = true;
+
final thread = _threadsByThreadId[threadId];
if (thread == null) {
throw DebugAdapterException('Thread $threadId was not found');
@@ -408,21 +417,18 @@
///
/// For [vm.EventKind.kPausePostRequest] which occurs after a restart, the
/// isolate will be re-configured (pause-exception behaviour, debuggable
- /// libraries, breakpoints) and then (if [resumeIfStarting] is `true`)
- /// resumed.
+ /// libraries, breakpoints) and then (if [autoResumeStartingIsolates] is
+ /// `true`) resumed.
///
- /// For [vm.EventKind.kPauseStart] and [resumeIfStarting] is `true`, the
- /// isolate will be resumed.
+ /// For [vm.EventKind.kPauseStart] and [autoResumeStartingIsolates] is `true`,
+ /// the isolate will be resumed.
///
/// For breakpoints with conditions that are not met and for logpoints, the
/// isolate will be automatically resumed.
///
/// For all other pause types, the isolate will remain paused and a
/// corresponding "Stopped" event sent to the editor.
- Future<void> _handlePause(
- vm.Event event, {
- bool resumeIfStarting = true,
- }) async {
+ Future<void> _handlePause(vm.Event event) async {
final eventKind = event.kind;
final isolate = event.isolate!;
final thread = _threadsByIsolateId[isolate.id!];
@@ -439,7 +445,7 @@
// after a hot restart.
if (eventKind == vm.EventKind.kPausePostRequest) {
await _configureIsolate(thread);
- if (resumeIfStarting) {
+ if (autoResumeStartingIsolates) {
await resumeThread(thread.threadId);
}
} else if (eventKind == vm.EventKind.kPauseStart) {
@@ -449,7 +455,7 @@
thread.startupHandled = true;
// If requested, automatically resume. Otherwise send a Stopped event to
// inform the client UI the thread is paused.
- if (resumeIfStarting) {
+ if (autoResumeStartingIsolates) {
await resumeThread(thread.threadId);
} else {
sendStoppedOnEntryEvent(thread.threadId);
