[dds] Improve suppression of auto-resuming threads when attaching to processes

Hopefully fixes #48274.

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

Author: DanTup

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

@@ -482,6 +482,10 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
     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 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
   /// 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 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
     await debuggerConnected(vmInfo);
 
     await _withErrorHandling(
-      () => _configureExistingIsolates(vmService, vmInfo, resumeIfStarting),
+      () => _configureExistingIsolates(vmService, vmInfo),
     );
 
     _debuggerInitializedCompleter.complete();
@@ -633,7 +635,6 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
   Future<void> _configureExistingIsolates(
     vm.VmService vmService,
     vm.VM vmInfo,
-    bool resumeIfStarting,
   ) async {
     final existingIsolateRefs = vmInfo.isolates;
     final existingIsolates = existingIsolateRefs != null
@@ -659,12 +660,11 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
       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);

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

@@ -94,7 +94,7 @@ class DartCliDebugAdapter extends DartDebugAdapter<DartLaunchRequestArguments,
     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 @@ class DartCliDebugAdapter extends DartDebugAdapter<DartLaunchRequestArguments,
         ? 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

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

@@ -79,7 +79,7 @@ class DartTestDebugAdapter extends DartDebugAdapter<DartLaunchRequestArguments,
     if (debug) {
       vmServiceInfoFile = generateVmServiceInfoFile();
       unawaited(waitForVmServiceInfoFile(logger, vmServiceInfoFile)
-          .then((uri) => connectDebugger(uri, resumeIfStarting: true)));
+          .then((uri) => connectDebugger(uri)));
     }
 
     final vmArgs = <String>[

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

@@ -56,6 +56,16 @@ class IsolateManager {
   /// [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 @@ class IsolateManager {
   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 @@ class IsolateManager {
     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 @@ class IsolateManager {
   /// [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 @@ class IsolateManager {
   ///
   /// 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 @@ class IsolateManager {
     // 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 @@ class IsolateManager {
         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);