[dds/dap] Ensure "terminated" event is sent when detaching from processes

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

pkg/dds/CHANGELOG.md

@@ -1,6 +1,7 @@
 # 2.5.0-dev
 - [DAP] `variables` requests now treat lists from `dart:typed_data` (such as `Uint8List`) like standard `List` instances and return their elements instead of class fields.
 - [DAP] `variables` requests now return information about the number of items in lists to allow the client to page through them.
+- [DAP] `terminated` events are now always sent when detaching whether or not the debuggee terminates after unpause.
 
 # 2.4.0
 - [DAP] Added support for sending progress notifications via `DartDebugAdapter.startProgressNotification`.

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

@@ -399,6 +399,13 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
   /// VM Service disconnects.
   bool isTerminating = false;
 
+  /// Whether or not the current termination is happening because the user
+  /// chose to detach from an attached process.
+  ///
+  /// This affects the message a user sees when the adapter shuts down ('exited'
+  /// vs 'detached').
+  bool isDetaching = false;
+
   /// Whether isolates that pause in the PauseExit state should be automatically
   /// resumed after any in-process log events have completed.
   ///
@@ -955,6 +962,13 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
     return shortError ?? rawError;
   }
 
+  /// Handles a detach request, removing breakpoints and unpausing paused
+  /// isolates.
+  Future<void> handleDetach() async {
+    isDetaching = true;
+    await preventBreakingAndResume();
+  }
+
   /// Sends a [TerminatedEvent] if one has not already been sent.
   ///
   /// Waits for any in-progress output events to complete first.
@@ -967,10 +981,12 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
 
     isTerminating = true;
     _hasSentTerminatedEvent = true;
+
     // Always add a leading newline since the last written text might not have
     // had one. Send directly via sendEvent and not sendOutput to ensure no
     // async since we're about to terminate.
-    sendEvent(OutputEventBody(output: '\nExited$exitSuffix.'));
+    final reason = isDetaching ? 'Detached' : 'Exited';
+    sendEvent(OutputEventBody(output: '\n$reason$exitSuffix.'));
     sendEvent(TerminatedEventBody());
   }
 
@@ -1276,8 +1292,9 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
   /// debugee if required.
   @nonVirtual
   Future<void> shutdown() async {
-    await _waitForPendingOutputEvents();
     await shutdownDebugee();
+    await _waitForPendingOutputEvents();
+    handleSessionTerminate();
 
     // Delay the shutdown slightly to allow any pending responses (such as the
     // terminate response) to be sent.

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

@@ -69,7 +69,7 @@ class DartCliDebugAdapter extends DartDebugAdapter<DartLaunchRequestArguments,
   /// app being run (or in the case of an attach, disconnect).
   Future<void> disconnectImpl() async {
     if (isAttach) {
-      await preventBreakingAndResume();
+      await handleDetach();
     }
     terminatePids(ProcessSignal.sigkill);
   }
@@ -289,7 +289,7 @@ class DartCliDebugAdapter extends DartDebugAdapter<DartLaunchRequestArguments,
   /// app being run (or in the case of an attach, disconnect).
   Future<void> terminateImpl() async {
     if (isAttach) {
-      await preventBreakingAndResume();
+      await handleDetach();
     }
     terminatePids(ProcessSignal.sigterm);
     await _process?.exitCode;

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

@@ -145,10 +145,18 @@ main() {
       final breakpointLine = lineWith(testFile, breakpointMarker);
       await dap.client.setBreakpoint(testFile, breakpointLine);
 
+      // Expect that termination is reported as 'Detached' when we explicitly
+      // requested a detach.
+      expect(dap.client.outputEvents.map((output) => output.output.trim()),
+          emits('Detached.'));
+
       // Detach using terminateRequest. Despite the name, terminateRequest is
       // the request for a graceful detach (and disconnectRequest is the
       // forceful shutdown).
-      await dap.client.terminate();
+      await Future.wait([
+        dap.client.event('terminated'),
+        dap.client.terminate(),
+      ]);
 
       // Expect the process terminates (and hasn't got stuck on the breakpoint
       // or exception).

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

@@ -32,6 +32,7 @@ main() {
             contains('"method":"streamListen","params":{"streamId":"Debug"}'),
           ),
         ),
+        dap.client.event('terminated'),
         dap.client.start(
           file: testFile,
           launch: () => dap.client.launch(
@@ -40,7 +41,6 @@ main() {
           ),
         ),
       ]);
-      await dap.client.terminate();
     });
 
     test('prints messages from dart:developer log()', () async {

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

@@ -33,12 +33,7 @@ final useInProcessDap = Platform.environment['DAP_TEST_INTERNAL'] == 'true';
 /// This is useful for debugging locally or on the bots and will include both
 /// DAP traffic (between the test DAP client and the DAP server) and the VM
 /// Service traffic (wrapped in a custom 'dart.log' event).
-///
-/// Verbose logging is temporarily enabled for all test runs to try and
-/// understand failures noted at https://github.com/dart-lang/sdk/issues/48274.
-/// Once resolved, this variable can be set back to the result of:
-///     Platform.environment['DAP_TEST_VERBOSE'] == 'true'
-final verboseLogging = true;
+final verboseLogging = Platform.environment['DAP_TEST_VERBOSE'] == 'true';
 
 /// A [RegExp] that matches the `path` part of a VM Service URI that contains
 /// an authentication token.