diff --git a/pkg/dds/analysis_options.yaml b/pkg/dds/analysis_options.yaml index 8f7435b60a3..59a93d3ab1a 100644 --- a/pkg/dds/analysis_options.yaml +++ b/pkg/dds/analysis_options.yaml @@ -6,3 +6,5 @@ linter: - directives_ordering - prefer_generic_function_type_aliases - prefer_relative_imports + - unnecessary_new + - unnecessary_const diff --git a/pkg/dds/lib/devtools_server.dart b/pkg/dds/lib/devtools_server.dart index 8949604c643..9d302638dc7 100644 --- a/pkg/dds/lib/devtools_server.dart +++ b/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 _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); diff --git a/pkg/dds/lib/src/devtools/client.dart b/pkg/dds/lib/src/devtools/client.dart index af82b55930b..629b9205809 100644 --- a/pkg/dds/lib/src/devtools/client.dart +++ b/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 implements StreamSink { 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 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 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 _firstResponsiveClient( + List 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().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 _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 ping() { + _devToolsPeer.sendNotification('ping'); + return _nextPingResponse.future; + } + /// Enable notifications to the user from this DevTools client. void enableNotifications() => _devToolsPeer.sendNotification('enableNotifications'); diff --git a/pkg/dds/test/devtools_server/instance_reuse_test.dart b/pkg/dds/test/devtools_server/instance_reuse_test.dart index 71d74285cec..84955ebc10a 100644 --- a/pkg/dds/test/devtools_server/instance_reuse_test.dart +++ b/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(