Remove spawning of DDS from DAP and let the spawned dart run/dart test do it

Fixes #56733

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

Author: DanTup

pkg/dartdev/lib/src/commands/debug_adapter.dart

@@ -34,14 +34,22 @@ class DebugAdapterCommand extends DartdevCommand {
         help: 'Whether to bind DAP/VM Service/DDS to IPv6 addresses.',
       )
       ..addFlag(
+        // Deprecated - DAP never spawns DDS now, but this flag is left
+        // temporarily to not crash clients passing it.
+        // TODO(dantup): Remove this after verifying nobody uses it.
         argDds,
         defaultsTo: true,
         help: 'Whether to enable DDS for debug sessions.',
+        hide: true,
       )
       ..addFlag(
+        // Deprecated - DAP never spawns DDS now, but this flag is left
+        // temporarily to not crash clients passing it.
+        // TODO(dantup): Remove this after verifying nobody uses it.
         argAuthCodes,
         defaultsTo: true,
         help: 'Whether to enable authentication codes for VM Services.',
+        hide: true,
       )
       ..addFlag(
         argTest,
@@ -60,8 +68,6 @@ class DebugAdapterCommand extends DartdevCommand {
       stdin,
       stdout.nonBlocking,
       ipv6: ipv6,
-      enableDds: args.flag(argDds),
-      enableAuthCodes: args.flag(argAuthCodes),
       test: args.flag(argTest),
       // Protocol errors should be written to stderr to help debug (or in the
       // case of a user running this command to explain it's for tools).

pkg/dds/CHANGELOG.md

@@ -1,4 +1,5 @@
 # 5.0.0-wip
+- [DAP] The debug adapter no longer spawns its own in-process copy of DDS, instead relying on one started by the Dart VM (or `Flutter`). This means the `enableDds` and `enableAuthCodes` arguments to the `DartDebugAdapter` base class have been deprecated and have any effect. Suppressing DDS (or auth codes) should be done in launch configuration (for example using `vmAdditionalArgs` or `toolArgs` depending on the target tool).
 - Updated the `devtools_shared` dependency to version `^11.0.0`.
 - Made `runDartDevelopmentServiceFromCLI` pass the specified bind address
   directly into `startDartDevelopmentService` without resolving the address.

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

@@ -15,7 +15,6 @@ import 'package:path/path.dart' as path;
 import 'package:vm_service/vm_service.dart' as vm;
 
 import '../../../dds.dart';
-import '../../../dds_launcher.dart';
 import '../../rpc_error_codes.dart';
 import '../base_debug_adapter.dart';
 import '../isolate_manager.dart';
@@ -354,12 +353,6 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
   /// value should contain trailing slashes.
   final orgDartlangSdkMappings = <String, Uri>{};
 
-  /// The DDS instance that was started and that [vmService] is connected to.
-  ///
-  /// `null` if the session is running in noDebug mode of the connection has not
-  /// yet been made or has been shut down.
-  DartDevelopmentServiceLauncher? _dds;
-
   /// The [DartInitializeRequestArguments] provided by the client in the
   /// `initialize` request.
   ///
@@ -369,12 +362,6 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
   /// Whether to use IPv6 for DAP/Debugger services.
   final bool ipv6;
 
-  /// Whether to enable DDS for launched applications.
-  final bool enableDds;
-
-  /// Whether to enable authentication codes for the VM Service/DDS.
-  final bool enableAuthCodes;
-
   /// A logger for printing diagnostic information.
   final Logger? logger;
 
@@ -491,8 +478,10 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
   DartDebugAdapter(
     ByteStreamServerChannel channel, {
     this.ipv6 = false,
-    this.enableDds = true,
-    this.enableAuthCodes = true,
+    @Deprecated('DAP never spawns DDS now, this `enableDds` does nothing')
+    bool enableDds = true,
+    @Deprecated('DAP never spawns DDS now, this `enableAuthCodes` does nothing')
+    bool enableAuthCodes = true,
     this.logger,
     Function? onError,
   }) : super(channel, onError: onError) {
@@ -649,59 +638,12 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
     }
   }
 
-  /// Attempts to start a DDS instance to connect to the VM Service at [uri].
-  ///
-  /// Returns the URI to connect the debugger to (whether it's a newly spawned
-  /// DDS or there was an existing one).
-  ///
-  /// If we failed to start DDS for a reason other than one already existed for
-  /// that VM Service we will return `null` and initiate a shutdown with the
-  /// exception printed to the user.
-  ///
-  /// If a new DDS instance was started, it is assigned to [_dds].
-  Future<Uri?> _startOrReuseDds(Uri uri) async {
-    try {
-      final dds = await startDds(uri);
-      _dds = dds;
-      return dds.wsUri;
-    } catch (error, stack) {
-      if (error is DartDevelopmentServiceException &&
-          error.errorCode ==
-              DartDevelopmentServiceException.existingDdsInstanceError) {
-        // If there's an existing DDS instance, we will just continue
-        // but need to map the URI to the ws: version.
-        return vmServiceUriToWebSocket(uri);
-      } else {
-        // Otherwise, we failed to start DDS for an unknown reason and
-        // consider this a fatal error. Handle terminating here (so we can
-        // print this error/stack)...
-        _handleDebuggerInitializationError(
-            'Failed to start DDS for $uri', error, stack);
-        // ... and return no URI as a signal to the caller.
-        return null;
-      }
-    }
-  }
-
   /// Connects to the VM Service at [uri] and initializes debugging.
   ///
   /// This is the implementation for [connectDebugger] which is executed in a
   /// try/catch.
   Future<void> _connectDebuggerImpl(Uri uri) async {
-    if (enableDds) {
-      // Start up a DDS instance for this VM.
-      logger?.call('Starting a DDS instance for $uri');
-      final targetUri = await _startOrReuseDds(uri);
-      if (targetUri == null) {
-        // If we got no URI, this is a fatal error and we can skip everything
-        // else. The detailed error would have been printed from
-        // [_startOrReuseDds].
-        return;
-      }
-      uri = targetUri;
-    } else {
-      uri = vmServiceUriToWebSocket(uri);
-    }
+    uri = vmServiceUriToWebSocket(uri);
 
     logger?.call('Connecting to debugger at $uri');
     sendConsoleOutput('Connecting to VM Service at $uri');
@@ -771,31 +713,6 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
     _debuggerInitializedCompleter.complete();
   }
 
-  /// Handlers an error during debugger initialization (such as exceptions
-  /// trying to call `getSupportedProtocols` or `getVM`) by sending it to the
-  /// client and shutting down.
-  ///
-  /// Without this, the exceptions may go unhandled and just terminate the debug
-  /// adapter, which may not be visible to the user. For example VS Code does
-  /// not expose stderr of a debug adapter process. With this change, the
-  /// exception will show up in the Debug Console before the debug session
-  /// terminates.
-  void _handleDebuggerInitializationError(
-      String reason, Object? error, StackTrace stack) {
-    final message = '$reason\n$error\n$stack';
-    logger?.call(message);
-    isTerminating = true;
-    sendConsoleOutput(message);
-    shutdown();
-  }
-
-  Future<DartDevelopmentServiceLauncher> startDds(Uri uri) {
-    return DartDevelopmentServiceLauncher.start(
-      remoteVmServiceUri: vmServiceUriToHttp(uri),
-      enableAuthCodes: enableAuthCodes,
-    );
-  }
-
   // This is intended for subclasses to override to provide a URI converter to
   // resolve package URIs to local paths.
   UriConverter? uriConverter() {
@@ -1070,7 +987,6 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
     isTerminating = true;
 
     await disconnectImpl();
-    await shutdownDebugee();
     sendResponse();
 
     await shutdown();
@@ -1688,25 +1604,11 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
     sendResponse(SetExceptionBreakpointsResponseBody());
   }
 
-  /// Shuts down/detaches from the debugee and cleans up.
-  ///
-  /// This is called by [disconnectRequest] and [terminateRequest] but may also
-  /// be called if the client just disconnects from the server without calling
-  /// either.
-  ///
-  /// This method must tolerate being called multiple times.
-  @nonVirtual
-  Future<void> shutdownDebugee() async {
-    await _dds?.shutdown();
-    _dds = null;
-  }
-
   /// Shuts down the debug adapter, including terminating/detaching from the
   /// debugee if required.
   @override
   @nonVirtual
   Future<void> shutdown() async {
-    await shutdownDebugee();
     await _waitForPendingOutputEvents();
     handleSessionTerminate();
 
@@ -2008,7 +1910,6 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
     isTerminating = true;
 
     await terminateImpl();
-    await shutdownDebugee();
     sendResponse();
 
     await shutdown();

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

@@ -30,8 +30,6 @@ class DartCliDebugAdapter extends DartDebugAdapter<DartLaunchRequestArguments,
   DartCliDebugAdapter(
     super.channel, {
     super.ipv6,
-    super.enableDds,
-    super.enableAuthCodes,
     super.logger,
     super.onError,
   });
@@ -106,12 +104,10 @@ class DartCliDebugAdapter extends DartDebugAdapter<DartLaunchRequestArguments,
 
     final vmArgs = <String>[
       ...?args.vmAdditionalArgs,
+      '--no-serve-devtools',
       if (debug) ...[
         '--enable-vm-service=${args.vmServicePort ?? 0}${ipv6 ? '/::1' : ''}',
-        if (!enableAuthCodes) '--disable-service-auth-codes'
       ],
-      '--disable-dart-dev',
-      '--no-dds',
       if (debug && vmServiceInfoFile != null) ...[
         '-DSILENT_VM_SERVICE=true',
         '--write-service-info=${Uri.file(vmServiceInfoFile.path)}'

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

@@ -29,8 +29,6 @@ class DartTestDebugAdapter extends DartDebugAdapter<DartLaunchRequestArguments,
   DartTestDebugAdapter(
     super.channel, {
     super.ipv6,
-    super.enableDds,
-    super.enableAuthCodes,
     super.logger,
     super.onError,
   });
@@ -83,7 +81,6 @@ class DartTestDebugAdapter extends DartDebugAdapter<DartLaunchRequestArguments,
       ...?args.vmAdditionalArgs,
       if (debug) ...[
         '--enable-vm-service=${args.vmServicePort ?? 0}${ipv6 ? '/::1' : ''}',
-        if (!enableAuthCodes) '--disable-service-auth-codes'
       ],
       if (debug && vmServiceInfoFile != null) ...[
         '-DSILENT_VM_SERVICE=true',
@@ -113,7 +110,6 @@ class DartTestDebugAdapter extends DartDebugAdapter<DartLaunchRequestArguments,
       // We should also ensure DDS is disabled (this may need a new flag since
       // we can't disable-dart-dev to get "dart test") and devtools is not
       // served.
-      // '--disable-dart-dev',
       'run',
       '--no-serve-devtools',
       'test:test',

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

@@ -37,7 +37,6 @@ class DdsHostedAdapter extends DartDebugAdapter<DartLaunchRequestArguments,
             (message) {},
           ),
           ipv6: true,
-          enableDds: false,
         );
 
   /// Whether the VM Service closing should be used as a signal to terminate the

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

@@ -20,17 +20,13 @@ class DapServer {
   final ByteStreamServerChannel channel;
   late final DartDebugAdapter adapter;
   final bool ipv6;
-  final bool enableDds;
-  final bool enableAuthCodes;
   final bool test;
   final Logger? logger;
 
   DapServer(
     Stream<List<int>> input,
     StreamSink<List<int>> output, {
     this.ipv6 = false,
-    this.enableDds = true,
-    this.enableAuthCodes = true,
     this.test = false,
     this.logger,
     Function? onError,
@@ -39,16 +35,12 @@ class DapServer {
         ? DartTestDebugAdapter(
             channel,
             ipv6: ipv6,
-            enableDds: enableDds,
-            enableAuthCodes: enableAuthCodes,
             logger: logger,
             onError: onError,
           )
         : DartCliDebugAdapter(
             channel,
             ipv6: ipv6,
-            enableDds: enableDds,
-            enableAuthCodes: enableAuthCodes,
             logger: logger,
             onError: onError,
           );

pkg/dds/test/dap/dart_cli_test.dart

@@ -87,7 +87,7 @@ main() {
 
         expect(adapter.executable, equals('/custom/dart'));
         // a standard built-in arg should be present.
-        expect(adapter.processArgs, contains('--disable-dart-dev'));
+        expect(adapter.processArgs, contains('--no-serve-devtools'));
       });
 
       test('with all args replaced', () async {
@@ -109,7 +109,9 @@ main() {
         expect(adapter.executable, equals('/custom/dart'));
         // normal built-in args are replaced by customToolReplacesArgs, but
         // user-provided toolArgs are not.
-        expect(adapter.processArgs, isNot(contains('--disable-dart-dev')));
+        // The argument here should match the one verified in the test above to
+        // ensure it's real.
+        expect(adapter.processArgs, isNot(contains('--no-serve-devtools')));
         expect(adapter.processArgs, contains('tool_args'));
       });
     });

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

@@ -128,7 +128,12 @@ main() {
 
       expect(
         outputEvents.map((e) => e.output).join(),
-        contains('Invalid --vm-service-uri argument: http://bogus.local'),
+        allOf(
+          contains(
+            'Failed to connect/initialize debugger for ws://bogus.local/',
+          ),
+          contains('Failed host lookup'),
+        ),
       );
     });
 

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

@@ -851,34 +851,34 @@ main() {
   }, timeout: Timeout.none);
 
   group('debug mode', () {
-    test(
-      'can run without DDS',
-      () async {
-        final dap = await DapTestSession.setUp(additionalArgs: ['--no-dds']);
-        addTearDown(dap.tearDown);
+    test('can be run without DDS using vmAdditionalArgs', () async {
+      final dap = await DapTestSession.setUp();
+      addTearDown(dap.tearDown);
 
-        final client = dap.client;
-        final testFile = dap.createTestFile(simpleBreakpointProgram);
-        final breakpointLine = lineWith(testFile, breakpointMarker);
+      final client = dap.client;
+      final testFile = dap.createTestFile(simpleBreakpointProgram);
+      final breakpointLine = lineWith(testFile, breakpointMarker);
 
-        await client.hitBreakpoint(testFile, breakpointLine);
+      await client.hitBreakpoint(testFile, breakpointLine,
+          vmAdditionalArgs: ['--no-dds']);
 
-        expect(await client.ddsAvailable, isFalse);
-      },
-    );
+      expect(await client.ddsAvailable, isFalse);
+    });
 
-    test('can run without auth codes', () async {
-      final dap =
-          await DapTestSession.setUp(additionalArgs: ['--no-auth-codes']);
+    test('can run without auth codes using vmAdditionalArgs', () async {
+      final dap = await DapTestSession.setUp();
       addTearDown(dap.tearDown);
 
       final testFile = dap.createTestFile(emptyProgram);
-      final outputEvents = await dap.client.collectOutput(file: testFile);
+      final outputEvents = await dap.client.collectOutput(
+        launch: () => dap.client.launch(testFile.path,
+            vmAdditionalArgs: ['--disable-service-auth-codes']),
+      );
       final vmServiceUri = _extractVmServiceUri(outputEvents.first);
       expect(vmServiceUri.path, isNot(matches(vmServiceAuthCodePathPattern)));
     });
 
-    test('can run with ipv6', () async {
+    test('can run with ipv6 with a DAP flag', () async {
       final dap = await DapTestSession.setUp(additionalArgs: ['--ipv6']);
       addTearDown(dap.tearDown);