[dds] Don't automatically resume when attaching to processes via DAP

Change-Id: I8b0fd4b36a9dd229226046a74e031de005908417
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/210920
Reviewed-by: Ben Konyi <bkonyi@google.com>
Commit-Queue: Ben Konyi <bkonyi@google.com>

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

@@ -467,11 +467,16 @@ abstract class DartDebugAdapter<TL extends DartLaunchRequestArguments,
   /// 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).
-  Future<void> connectDebugger(Uri uri) async {
+  ///
+  /// 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,
+  }) async {
     // Start up a DDS instance for this VM.
     if (enableDds) {
-      // TODO(dantup): Do we need to worry about there already being one connected
-      //   if this URL came from another service that may have started one?
       logger?.call('Starting a DDS instance for $uri');
       try {
         final dds = await DartDevelopmentService.startDartDevelopmentService(
@@ -552,7 +557,8 @@ abstract class DartDebugAdapter<TL extends DartLaunchRequestArguments,
       final pauseEventKind = isolate.runnable ?? false
           ? vm.EventKind.kIsolateRunnable
           : vm.EventKind.kIsolateStart;
-      await _isolateManager.registerIsolate(isolate, pauseEventKind);
+      final thread =
+          await _isolateManager.registerIsolate(isolate, pauseEventKind);
 
       // If the Isolate already has a Pause event we can give it to the
       // IsolateManager to handle (if it's PausePostStart it will re-configure
@@ -560,9 +566,18 @@ abstract class DartDebugAdapter<TL extends DartLaunchRequestArguments,
       // runnable - otherwise we'll handle this when it becomes runnable in an
       // event later).
       if (isolate.pauseEvent?.kind?.startsWith('Pause') ?? false) {
-        await _isolateManager.handleEvent(isolate.pauseEvent!);
+        await _isolateManager.handleEvent(
+          isolate.pauseEvent!,
+          resumeIfStarting: resumeIfStarting,
+        );
       } else if (isolate.runnable == true) {
-        await _isolateManager.resumeIsolate(isolate);
+        // If requested, automatically resume. Otherwise send a Stopped event to
+        // inform the client UI the thread is paused.
+        if (resumeIfStarting) {
+          await _isolateManager.resumeIsolate(isolate);
+        } else {
+          _isolateManager.sendStoppedOnEntryEvent(thread.threadId);
+        }
       }
     }));
 

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

@@ -140,7 +140,7 @@ class DartCliDebugAdapter extends DartDebugAdapter<DartLaunchRequestArguments,
 
       vmServiceInfoFile = File(serviceInfoFilePath);
       unawaited(waitForVmServiceInfoFile(vmServiceInfoFile)
-          .then((uri) => connectDebugger(uri)));
+          .then((uri) => connectDebugger(uri, resumeIfStarting: true)));
     }
 
     final vmArgs = <String>[
@@ -230,7 +230,7 @@ class DartCliDebugAdapter extends DartDebugAdapter<DartLaunchRequestArguments,
         ? Uri.parse(vmServiceUri)
         : await waitForVmServiceInfoFile(File(vmServiceInfoFile!));
 
-    unawaited(connectDebugger(uri));
+    unawaited(connectDebugger(uri, resumeIfStarting: false));
   }
 
   /// Calls the client (via a `runInTerminal` request) to spawn the process so

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

@@ -128,8 +128,14 @@ class IsolateManager {
 
   ThreadInfo? getThread(int threadId) => _threadsByThreadId[threadId];
 
-  /// Handles Isolate and Debug events
-  Future<void> handleEvent(vm.Event event) async {
+  /// 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 {
     final isolateId = event.isolate?.id;
     if (isolateId == null) {
       return;
@@ -151,7 +157,7 @@ class IsolateManager {
     if (eventKind == vm.EventKind.kIsolateExit) {
       _handleExit(event);
     } else if (eventKind?.startsWith('Pause') ?? false) {
-      await _handlePause(event);
+      await _handlePause(event, resumeIfStarting: resumeIfStarting);
     } else if (eventKind == vm.EventKind.kResume) {
       _handleResumed(event);
     }
@@ -163,7 +169,7 @@ class IsolateManager {
   /// New isolates will be configured with the correct pause-exception behaviour,
   /// libraries will be marked as debuggable if appropriate, and breakpoints
   /// sent.
-  Future<void> registerIsolate(
+  Future<ThreadInfo> registerIsolate(
     vm.IsolateRef isolate,
     String eventKind,
   ) async {
@@ -193,6 +199,8 @@ class IsolateManager {
       await _configureIsolate(isolate);
       registrationCompleter.complete();
     }
+
+    return info;
   }
 
   Future<void> resumeIsolate(vm.IsolateRef isolateRef,
@@ -245,6 +253,11 @@ class IsolateManager {
     }
   }
 
+  /// Sends an event informing the client that a thread is stopped at entry.
+  void sendStoppedOnEntryEvent(int threadId) {
+    _adapter.sendEvent(StoppedEventBody(reason: 'entry', threadId: threadId));
+  }
+
   /// Records breakpoints for [uri].
   ///
   /// [breakpoints] represents the new set and entirely replaces anything given
@@ -368,18 +381,23 @@ class IsolateManager {
 
   /// Handles a pause event.
   ///
-  /// For [vm.EventKind.kPausePostRequest] which occurs after a restart, the isolate
-  /// will be re-configured (pause-exception behaviour, debuggable libraries,
-  /// breakpoints) and then resumed.
+  /// 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.
   ///
-  /// For [vm.EventKind.kPauseStart], the isolate will be resumed.
+  /// For [vm.EventKind.kPauseStart] and [resumeIfStarting] 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) async {
+  Future<void> _handlePause(
+    vm.Event event, {
+    bool resumeIfStarting = true,
+  }) async {
     final eventKind = event.kind;
     final isolate = event.isolate!;
     final thread = _threadsByIsolateId[isolate.id!];
@@ -396,13 +414,21 @@ class IsolateManager {
     // after a hot restart.
     if (eventKind == vm.EventKind.kPausePostRequest) {
       await _configureIsolate(isolate);
-      await resumeThread(thread.threadId);
+      if (resumeIfStarting) {
+        await resumeThread(thread.threadId);
+      }
     } else if (eventKind == vm.EventKind.kPauseStart) {
       // Don't resume from a PauseStart if this has already happened (see
       // comments on [thread.hasBeenStarted]).
       if (!thread.hasBeenStarted) {
-        thread.hasBeenStarted = true;
-        await resumeThread(thread.threadId);
+        // If requested, automatically resume. Otherwise send a Stopped event to
+        // inform the client UI the thread is paused.
+        if (resumeIfStarting) {
+          thread.hasBeenStarted = true;
+          await resumeThread(thread.threadId);
+        } else {
+          sendStoppedOnEntryEvent(thread.threadId);
+        }
       }
     } else {
       // PauseExit, PauseBreakpoint, PauseInterrupted, PauseException

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

@@ -31,6 +31,7 @@ main() {
       final outputEvents = await dap.client.collectOutput(
         launch: () => dap.client.attach(
           vmServiceUri: vmServiceUri.toString(),
+          autoResume: true,
           cwd: dap.testAppDir.path,
         ),
       );
@@ -81,6 +82,7 @@ main() {
       final outputEvents = await dap.client.collectOutput(
         launch: () => dap.client.attach(
           vmServiceInfoFile: vmServiceInfoFilePath,
+          autoResume: true,
           cwd: dap.testAppDir.path,
         ),
       );

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

@@ -62,6 +62,7 @@ class DapTestClient {
   /// Send an attachRequest to the server, asking it to attach to an existing
   /// Dart program.
   Future<Response> attach({
+    required bool autoResume,
     String? vmServiceUri,
     String? vmServiceInfoFile,
     String? cwd,
@@ -70,12 +71,20 @@ class DapTestClient {
     bool? debugExternalPackageLibraries,
     bool? evaluateGettersInDebugViews,
     bool? evaluateToStringInDebugViews,
-  }) {
+  }) async {
     assert(
       (vmServiceUri == null) != (vmServiceInfoFile == null),
       'Provide exactly one of vmServiceUri/vmServiceInfoFile',
     );
-    return sendRequest(
+
+    // When attaching, the paused VM will not be automatically unpaused, but
+    // instead send a Stopped(reason: 'entry') event. Respond to this by
+    // resuming.
+    final resumeFuture = autoResume
+        ? expectStop('entry').then((event) => continue_(event.threadId!))
+        : null;
+
+    final attachResponse = sendRequest(
       DartAttachRequestArguments(
         vmServiceUri: vmServiceUri,
         vmServiceInfoFile: vmServiceInfoFile,
@@ -94,6 +103,11 @@ class DapTestClient {
       // (DartAttachRequestArguments).
       overrideCommand: 'attach',
     );
+
+    // If we were expecting a pause and to resume, ensure that happens.
+    await resumeFuture;
+
+    return attachResponse;
   }
 
   /// Sends a continue request for the given thread.
@@ -553,17 +567,20 @@ extension DapTestClientExtension on DapTestClient {
 
     final result =
         await getValidStack(stop.threadId!, startFrame: 0, numFrames: 1);
-    expect(result.stackFrames, hasLength(1));
-    final frame = result.stackFrames[0];
 
-    if (file != null) {
-      expect(frame.source?.path, equals(file.path));
-    }
-    if (sourceName != null) {
-      expect(frame.source?.name, equals(sourceName));
-    }
-    if (line != null) {
-      expect(frame.line, equals(line));
+    if (file != null || line != null || sourceName != null) {
+      expect(result.stackFrames, hasLength(1));
+      final frame = result.stackFrames[0];
+
+      if (file != null) {
+        expect(frame.source?.path, equals(file.path));
+      }
+      if (sourceName != null) {
+        expect(frame.source?.name, equals(sourceName));
+      }
+      if (line != null) {
+        expect(frame.line, equals(line));
+      }
     }
 
     return stop;