[devtools] Ping browsers running DevTools before trying to reuse them

It's possible the browser has been closed but is in the SSE timeout period and therefore looks active. Ping it to see if it's actually responsive before deciding whether to reuse it or launch a new window.

Fixes https://github.com/Dart-Code/Dart-Code/issues/3966.

Change-Id: I2fdcba036b8b63f7ab974e8fef5dd565c2917b64
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/257581
Reviewed-by: Kenzie Davisson <kenzieschmoll@google.com>
Commit-Queue: Ben Konyi <bkonyi@google.com>
Reviewed-by: Ben Konyi <bkonyi@google.com>

pkg/dds/analysis_options.yaml

@@ -6,3 +6,5 @@ linter:
     - directives_ordering
     - prefer_generic_function_type_aliases
     - prefer_relative_imports
+    - unnecessary_new
+    - unnecessary_const

pkg/dds/lib/devtools_server.dart

@@ -507,7 +507,7 @@ class DevToolsServer {
         params.containsKey('notify') && params['notify'] == true;
     final page = params['page'];
     if (canReuse &&
-        _tryReuseExistingDevToolsInstance(
+        await _tryReuseExistingDevToolsInstance(
           vmServiceUri,
           page,
           shouldNotify,
@@ -586,15 +586,20 @@ class DevToolsServer {
     print('Writing memory profile samples to $profileFile...');
   }
 
-  bool _tryReuseExistingDevToolsInstance(
+  /// Tries to reuse an existing DevTools instance.
+  ///
+  /// Because SSE connections have timeouts that prevent us knowing if a client
+  /// has really gone away for up to 30s, this method will attempt to ping
+  /// candidate first to see if it is still responsive.
+  Future<bool> _tryReuseExistingDevToolsInstance(
     Uri vmServiceUri,
     String? page,
     bool notifyUser,
-  ) {
+  ) async {
     // First try to find a client that's already connected to this VM service,
     // and just send the user a notification for that one.
     final existingClient =
-        clientManager.findExistingConnectedReusableClient(vmServiceUri);
+        await clientManager.findExistingConnectedReusableClient(vmServiceUri);
     if (existingClient != null) {
       try {
         if (page != null) {
@@ -610,7 +615,7 @@ class DevToolsServer {
       }
     }
 
-    final reusableClient = clientManager.findReusableClient();
+    final reusableClient = await clientManager.findReusableClient();
     if (reusableClient != null) {
       try {
         reusableClient.connectToVmService(vmServiceUri, notifyUser);

pkg/dds/lib/src/devtools/client.dart

@@ -4,7 +4,6 @@
 
 import 'dart:async';
 
-import 'package:collection/collection.dart';
 import 'package:devtools_shared/devtools_server.dart';
 import 'package:json_rpc_2/src/peer.dart' as json_rpc;
 import 'package:meta/meta.dart';
@@ -46,6 +45,18 @@ class LoggingMiddlewareSink<S> implements StreamSink<S> {
 class ClientManager {
   ClientManager({required this.requestNotificationPermissions});
 
+  /// The timeout to wait for a ping when verifying a client is still
+  /// responsive.
+  ///
+  /// Usually clients are removed from the set once they disconnect, but due
+  /// to SSE timeouts (to handle proxies dropping connections) a client may
+  /// appear connected for up to 30s after they disconnected.
+  ///
+  /// In cases where we need to know quickly if a client is active (for example
+  /// when trying to reuse it), we will ping it and wait only this period to see
+  /// if it responds before assuming it is not available.
+  static const _clientResponsivenessTimeout = Duration(milliseconds: 500);
+
   /// Whether to immediately request notification permissions when a client connects.
   /// Otherwise permission will be requested only with the first notification.
   final bool requestNotificationPermissions;
@@ -67,22 +78,58 @@ class ClientManager {
   /// a VM service that we can reuse (for example if a user stopped debugging
   /// and it disconnected, then started debugging again, we want to reuse
   /// the open DevTools window).
-  DevToolsClient? findReusableClient() {
-    return _clients.firstWhereOrNull(
-      (c) => !c.hasConnection && !c.embedded,
-    );
+  ///
+  /// Candidate clients will be pinged first to ensure they are responsive, to
+  /// avoid trying to reuse a client that is in an SSE timeout where we don't
+  /// know if it is refreshing or gone.
+  Future<DevToolsClient?> findReusableClient() {
+    final candidates =
+        _clients.where((c) => !c.hasConnection && !c.embedded).toList();
+
+    return _firstResponsiveClient(candidates);
   }
 
   /// Finds a client that may already be connected to this VM Service.
-  DevToolsClient? findExistingConnectedReusableClient(Uri vmServiceUri) {
-    // Checking the whole URI will fail if DevTools converted it from HTTP to
-    // WS, so just check the host, port and first segment of path (token).
-    return _clients.firstWhereOrNull(
-      (c) =>
-          c.hasConnection &&
-          !c.embedded &&
-          _areSameVmServices(c.vmServiceUri!, vmServiceUri),
+  ///
+  /// Candidate clients will be pinged first to ensure they are responsive, to
+  /// avoid trying to reuse a client that is in an SSE timeout where we don't
+  /// know if it is refreshing or gone.
+  Future<DevToolsClient?> findExistingConnectedReusableClient(
+      Uri vmServiceUri) {
+    final candidates = _clients
+        .where((c) =>
+            c.hasConnection &&
+            !c.embedded &&
+            _areSameVmServices(c.vmServiceUri!, vmServiceUri))
+        .toList();
+
+    return _firstResponsiveClient(candidates);
+  }
+
+  /// Pings [candidates] and returns the first one that responds.
+  ///
+  /// If no clients respond within a short period, returns `null` because all
+  /// clients have likely been closed (but are in an SSE timeout period).
+  Future<DevToolsClient?> _firstResponsiveClient(
+      List<DevToolsClient> candidates) async {
+    if (candidates.isEmpty) {
+      return null;
+    }
+    // Use `Future.any` to get the first client that responds to its
+    // ping.
+    final firstRespondingClient = Future.any(
+      candidates.cast<DevToolsClient?>().map((client) async {
+        await client?.ping();
+        return client;
+      }),
     );
+
+    final client = await firstRespondingClient.timeout(
+      _clientResponsivenessTimeout,
+      onTimeout: () => null,
+    );
+
+    return client;
   }
 
   @override
@@ -100,6 +147,10 @@ class ClientManager {
         }
       };
 
+  /// Checks whether two VM Services are equiavlent.
+  ///
+  /// Checking the whole URI will fail if DevTools converted it from HTTP to
+  /// WS, so just checks the host, port and first segment of path (token).
   bool _areSameVmServices(Uri uri1, Uri uri2) {
     return uri1.host == uri2.host &&
         uri1.port == uri2.port &&
@@ -171,8 +222,18 @@ class DevToolsClient {
       final value = parameters['value'].value;
       ServerApi.devToolsPreferences.properties[key] = value;
     });
+
+    _devToolsPeer.registerMethod('pingResponse', (parameters) {
+      _nextPingResponse.complete();
+      _nextPingResponse = Completer();
+    });
   }
 
+  /// A completer that completes when the client next responds to a ping.
+  ///
+  /// See [ping].
+  Completer<void> _nextPingResponse = Completer();
+
   /// Notify the DevTools client to connect to a specific VM service instance.
   void connectToVmService(Uri uri, bool notifyUser) {
     _devToolsPeer.sendNotification('connectToVm', {
@@ -183,6 +244,12 @@ class DevToolsClient {
 
   void notify() => _devToolsPeer.sendNotification('notify');
 
+  /// Pings the client and waits for it to respond.
+  Future<void> ping() {
+    _devToolsPeer.sendNotification('ping');
+    return _nextPingResponse.future;
+  }
+
   /// Enable notifications to the user from this DevTools client.
   void enableNotifications() =>
       _devToolsPeer.sendNotification('enableNotifications');

pkg/dds/test/devtools_server/instance_reuse_test.dart

@@ -100,6 +100,53 @@ void main() {
         }
       }, timeout: const Timeout.factor(20));
 
+      test('does not reuse if browser was terminated (unresponsive ping)',
+          () async {
+        // Register the VM.
+        await testController.send(
+          'vm.register',
+          {'uri': testController.appFixture.serviceUri.toString()},
+        );
+
+        // Spawn a DevTools in a browser.
+        final event = await testController.serverStartedEvent.future;
+        final devToolsUri =
+            'http://${event['params']['host']}:${event['params']['port']}';
+        final launchUrl = '$devToolsUri/?page=logging'
+            '&uri=${Uri.encodeQueryComponent(testController.appFixture.serviceUri.toString())}';
+        final chrome = await Chrome.locate()!.start(url: launchUrl);
+        try {
+          {
+            final serverResponse = await testController.waitForClients(
+              requiredConnectionState: true,
+            );
+            expect(serverResponse['clients'], hasLength(1));
+          }
+
+          // Terminate the browser to prevent it responding to future pings.
+          chrome.kill();
+
+          // Send a request to the server to launch and ensure it did
+          // not reuse the existing connection. Launch it on a different page
+          // so we can easily tell once this one has connected.
+          final launchResponse = await testController.sendLaunchDevToolsRequest(
+            useVmService: useVmService,
+            reuseWindows: true,
+            page: 'memory',
+          );
+          expect(launchResponse['reused'], isFalse);
+
+          // Ensure there's now two connections.
+          final serverResponse = await testController.waitForClients(
+            requiredConnectionState: true,
+            requiredPage: 'memory',
+          );
+          expect(serverResponse['clients'], hasLength(2));
+        } finally {
+          chrome.kill();
+        }
+      }, timeout: const Timeout.factor(20));
+
       test('reuses DevTools instance if not connected to a VM', () async {
         // Register the VM.
         await testController.send(