[dds/dap] Handle errors during VM Service connection/initialization and ensure they're reported to the user

Sometimes we see unhandled exceptions during this setup work, see:

- flutter/flutter#148346
- flutter/flutter#148348

Currently these are unhandled and bring the debug adapter down. In VS Code, the error (send to stderr) is not visible to the user so they just see a silent crash which makes it very difficult to report (and the Flutter crash reports don't have any context).

This change will send the exception to the client and then cleanly terminate, which should give the user more information to open a good bug report (assuming the issue wasn't just something like them terminating the app as it was starting).

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

Author: DanTup

pkg/dds/CHANGELOG.md

@@ -1,3 +1,6 @@
+# 4.2.2
+- [DAP] Exceptions that occur while the debug adapter is connecting to the VM Service and configuring isolates will no longer cause the debug adapter to terminate. Instead, the errors are reporting via a `console` `OutputEvent` and the adapter will shut down gracefully.
+
 # 4.2.1
 - [DAP]: Fixed an issue where breakpoint `changed` events might contain incorrect location information when new isolates are created, causing breakpoints to appear to move in the editor.
 - [DAP]: For consistency with other values, automatic `toString()` invocations for debugger views no longer expand long strings and instead show truncated values. Full values continue to be returned for evaluation (`context=="repl"`) and when copying to the clipboard (`context=="clipboard"`).

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

@@ -623,25 +623,86 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
   /// The URI protocol will be changed to ws/wss but otherwise not normalized.
   /// The caller should handle any other normalisation (such as adding /ws to
   /// the end if required).
+  ///
+  /// The implementation for this method is run in try/catch and any
+  /// exceptions during initialization will result in the debug adapter
+  /// reporting an error to the user and shutting down.
   Future<void> connectDebugger(Uri uri) async {
-    // Start up a DDS instance for this VM.
-    if (enableDds) {
-      logger?.call('Starting a DDS instance for $uri');
-      try {
+    try {
+      await _connectDebuggerImpl(uri);
+    } catch (error, stack) {
+      final message = 'Failed to connect/initialize debugger for $uri:\n'
+          '$error\n$stack';
+      logger?.call(message);
+      sendConsoleOutput(message);
+      shutdown();
+    }
+  }
+
+  /// 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) {
+    // Use a completer to handle DDS because `startDds` (and therefore
+    // `runZonedGuarded`) may never complete when there's an existing DDS
+    // (see https://github.com/dart-lang/sdk/issues/55731#issuecomment-2114699898)
+    final completer = Completer<Uri?>();
+    runZonedGuarded(
+      () async {
         final dds = await startDds(uri, uriConverter());
         _dds = dds;
-        uri = dds.wsUri!;
-      } on DartDevelopmentServiceException catch (e) {
-        // If there's already a DDS instance, then just continue. This is common
-        // when attaching, as the program may have already been run with a DDS
-        // instance.
-        if (e.errorCode ==
-            DartDevelopmentServiceException.existingDdsInstanceError) {
-          uri = vmServiceUriToWebSocket(uri);
+        if (!completer.isCompleted) {
+          completer.complete(dds.wsUri!);
+        }
+      },
+      (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.
+          if (!completer.isCompleted) {
+            completer.complete(vmServiceUriToWebSocket(uri));
+          }
         } else {
-          rethrow;
+          // 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 complete with no URI as a signal to the caller.
+          if (!completer.isCompleted) {
+            completer.complete(null);
+          }
         }
+      },
+    );
+    return completer.future;
+  }
+
+  /// 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);
     }
@@ -714,6 +775,33 @@ 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<DartDevelopmentService> startDds(Uri uri, UriConverter? uriConverter) {
+    return DartDevelopmentService.startDartDevelopmentService(
+      vmServiceUriToHttp(uri),
+      enableAuthCodes: enableAuthCodes,
+      ipv6: ipv6,
+      uriConverter: uriConverter,
+    );
+  }
+
   // This is intended for subclasses to override to provide a URI converter to
   // resolve package URIs to local paths.
   UriConverter? uriConverter() {
@@ -732,15 +820,6 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
     );
   }
 
-  Future<DartDevelopmentService> startDds(Uri uri, UriConverter? uriConverter) {
-    return DartDevelopmentService.startDartDevelopmentService(
-      vmServiceUriToHttp(uri),
-      enableAuthCodes: enableAuthCodes,
-      ipv6: ipv6,
-      uriConverter: uriConverter,
-    );
-  }
-
   /// Starts reporting progress to the client for a single operation.
   ///
   /// The returned [DapProgressReporter] can be used to send updated messages

pkg/dds/pubspec.yaml

@@ -1,5 +1,5 @@
 name: dds
-version: 4.2.1
+version: 4.2.2
 description: >-
   A library used to spawn the Dart Developer Service, used to communicate with
   a Dart VM Service instance.

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

@@ -38,32 +38,28 @@ main() {
         ),
       );
 
-      // Expect a "console" output event that prints the URI of the VM Service
-      // the debugger connects to.
-      final vmConnection = outputEvents.first;
-      expect(vmConnection.output,
-          startsWith('Connecting to VM Service at ws://127.0.0.1:'));
-      expect(vmConnection.category, anyOf('console', isNull));
-
-      // Expect the normal applications output.
-      final output = outputEvents
-          .skip(2)
-          .map((e) => e.output)
-          // The stdout also contains the VM Service+DevTools banners.
-          .where(
-            (line) =>
-                !line.startsWith('The Dart VM service is listening on') &&
-                !line.startsWith(
-                    'The Dart DevTools debugger and profiler is available at'),
-          )
-          .join();
-      expectLines(output, [
+      expectLines(outputEvents.map((output) => output.output).join(), [
+        startsWith('Connecting to VM Service at ws://127.0.0.1:'),
+        'Connected to the VM Service.',
+        startsWith('The Dart VM service is listening on'),
+        startsWith('The Dart DevTools debugger and profiler is available at'),
         'Hello!',
         'World!',
         'args: [one, two]',
         '',
         'Exited.',
       ]);
+
+      // Ensure the categories were set correctly.
+      for (final output in outputEvents) {
+        if (output.output.contains('VM Service') ||
+            output.output.contains('Exited')) {
+          expect(output.category, anyOf('console', isNull));
+        } else {
+          // User output.
+          expect(output.category, 'stdout');
+        }
+      }
     });
 
     test('can attach to a simple script using vmServiceInfoFile', () async {
@@ -91,34 +87,47 @@ main() {
         ),
       );
 
-      // Expect a "console" output event that prints the URI of the VM Service
-      // the debugger connects to.
-      final vmConnection = outputEvents.first;
-      expect(
-        vmConnection.output,
+      expectLines(outputEvents.map((output) => output.output).join(), [
         startsWith('Connecting to VM Service at ws://127.0.0.1:'),
-      );
-      expect(vmConnection.category, anyOf('console', isNull));
-
-      // Expect the normal applications output.
-      final output = outputEvents
-          .skip(2)
-          .map((e) => e.output)
-          // The stdout also contains the VM Service+DevTools banners.
-          .where(
-            (line) =>
-                !line.startsWith('The Dart VM service is listening on') &&
-                !line.startsWith(
-                    'The Dart DevTools debugger and profiler is available at'),
-          )
-          .join();
-      expectLines(output, [
+        'Connected to the VM Service.',
+        startsWith('The Dart VM service is listening on'),
+        startsWith('The Dart DevTools debugger and profiler is available at'),
         'Hello!',
         'World!',
         'args: [one, two]',
         '',
         'Exited.',
       ]);
+
+      // Ensure the categories were set correctly.
+      for (final output in outputEvents) {
+        if (output.output.contains('VM Service') ||
+            output.output.contains('Exited')) {
+          expect(output.category, anyOf('console', isNull));
+        } else {
+          // User output.
+          expect(output.category, 'stdout');
+        }
+      }
+    });
+
+    test('reports initialization failures if can\t connect to the VM Service',
+        () async {
+      final outputEvents = await dap.client.collectOutput(
+        launch: () => dap.client.attach(
+          vmServiceUri: 'ws://bogus.local/',
+          autoResumeOnEntry: false,
+          autoResumeOnExit: false,
+        ),
+      );
+
+      expect(
+        outputEvents.map((e) => e.output).join(),
+        allOf(
+          contains('Failed to start DDS for ws://bogus.local/'),
+          contains('Failed host lookup'),
+        ),
+      );
     });
 
     test('removes breakpoints/pause and resumes on detach', () async {