From 44f869d3a6ff6902243f65987c052d117880a43d Mon Sep 17 00:00:00 2001 From: Danny Tuppeny Date: Fri, 12 Jun 2020 14:11:24 +0000 Subject: [PATCH] Cap the amount of time spent waiting for plugin completions in LSP Change-Id: Ic60d9936a252a1322becc233d696dfed41af834a Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/150927 Commit-Queue: Danny Tuppeny Reviewed-by: Brian Wilkerson --- .../lib/src/domain_completion.dart | 4 +- .../src/lsp/handlers/handler_completion.dart | 7 +-- .../lib/src/lsp/handlers/handlers.dart | 9 +++- .../test/lsp/completion_test.dart | 43 +++++++++++++++++++ .../test/lsp/server_abstract.dart | 4 +- pkg/analysis_server/test/mocks.dart | 23 +++++++++- 6 files changed, 81 insertions(+), 9 deletions(-) diff --git a/pkg/analysis_server/lib/src/domain_completion.dart b/pkg/analysis_server/lib/src/domain_completion.dart index d9ffacaf6c0..1c639420bfc 100644 --- a/pkg/analysis_server/lib/src/domain_completion.dart +++ b/pkg/analysis_server/lib/src/domain_completion.dart @@ -111,11 +111,11 @@ class CompletionDomainHandler extends AbstractRequestHandler { // false) then send empty results // - // Add the fixes produced by plugins to the server-generated fixes. + // Add the completions produced by plugins to the server-generated list. // if (pluginFutures != null) { var responses = await waitForResponses(pluginFutures, - requestParameters: requestParams); + requestParameters: requestParams, timeout: 100); for (var response in responses) { var result = plugin.CompletionGetSuggestionsResult.fromResponse(response); diff --git a/pkg/analysis_server/lib/src/lsp/handlers/handler_completion.dart b/pkg/analysis_server/lib/src/lsp/handlers/handler_completion.dart index b0f186b4118..d9c181564da 100644 --- a/pkg/analysis_server/lib/src/lsp/handlers/handler_completion.dart +++ b/pkg/analysis_server/lib/src/lsp/handlers/handler_completion.dart @@ -25,8 +25,8 @@ import 'package:analyzer/src/services/available_declarations.dart'; import 'package:analyzer_plugin/protocol/protocol_common.dart'; import 'package:analyzer_plugin/protocol/protocol_generated.dart' as plugin; -// If the client does not provide capabilities.completion.completionItemKind.valueSet -// then we must never send a kind that's not in this list. +/// If the client does not provide capabilities.completion.completionItemKind.valueSet +/// then we must never send a kind that's not in this list. final defaultSupportedCompletionKinds = HashSet.of([ CompletionItemKind.Text, CompletionItemKind.Method, @@ -163,7 +163,8 @@ class CompletionHandler int offset, ) async { final requestParams = plugin.CompletionGetSuggestionsParams(path, offset); - final pluginResponses = await requestFromPlugins(path, requestParams); + final pluginResponses = + await requestFromPlugins(path, requestParams, timeout: 100); final pluginResults = pluginResponses .map((e) => plugin.CompletionGetSuggestionsResult.fromResponse(e)) diff --git a/pkg/analysis_server/lib/src/lsp/handlers/handlers.dart b/pkg/analysis_server/lib/src/lsp/handlers/handlers.dart index 55b9980a7ef..c87b9c7a7b8 100644 --- a/pkg/analysis_server/lib/src/lsp/handlers/handlers.dart +++ b/pkg/analysis_server/lib/src/lsp/handlers/handlers.dart @@ -82,12 +82,17 @@ mixin Handler { mixin LspPluginRequestHandlerMixin on RequestHandlerMixin { - Future> requestFromPlugins(String path, RequestParams params) { + Future> requestFromPlugins( + String path, + RequestParams params, { + int timeout = 500, + }) { final driver = server.getAnalysisDriver(path); final pluginFutures = server.pluginManager .broadcastRequest(params, contextRoot: driver.contextRoot); - return waitForResponses(pluginFutures, requestParameters: params); + return waitForResponses(pluginFutures, + requestParameters: params, timeout: timeout); } } diff --git a/pkg/analysis_server/test/lsp/completion_test.dart b/pkg/analysis_server/test/lsp/completion_test.dart index 21c1a89b9df..59b90956330 100644 --- a/pkg/analysis_server/test/lsp/completion_test.dart +++ b/pkg/analysis_server/test/lsp/completion_test.dart @@ -192,6 +192,49 @@ class CompletionTest extends AbstractLspAnalysisServerTest { expect(suggestion.label, equals('id')); } + Future test_fromPlugin_tooSlow() async { + final content = ''' + void main() { + var x = ''; + print(^); + } + '''; + + final pluginResult = plugin.CompletionGetSuggestionsResult( + content.indexOf('^'), + 0, + [ + plugin.CompletionSuggestion( + plugin.CompletionSuggestionKind.INVOCATION, + 100, + 'x.toUpperCase()', + -1, + -1, + false, + false, + ), + ], + ); + configureTestPlugin( + respondWith: pluginResult, + // Don't respond within an acceptable time + respondAfter: Duration(seconds: 1), + ); + + await initialize(); + await openFile(mainFileUri, withoutMarkers(content)); + + final res = await getCompletion(mainFileUri, positionFromMarker(content)); + final fromServer = res.singleWhere((c) => c.label == 'x'); + final fromPlugin = res.singleWhere((c) => c.label == 'x.toUpperCase()', + orElse: () => null); + + // Server results should still be included. + expect(fromServer.kind, equals(CompletionItemKind.Variable)); + // Plugin results are not because they didn't arrive in time. + expect(fromPlugin, isNull); + } + Future test_gettersAndSetters() async { final content = ''' class MyClass { diff --git a/pkg/analysis_server/test/lsp/server_abstract.dart b/pkg/analysis_server/test/lsp/server_abstract.dart index 3498d9ed74c..bbf3ca111a2 100644 --- a/pkg/analysis_server/test/lsp/server_abstract.dart +++ b/pkg/analysis_server/test/lsp/server_abstract.dart @@ -52,13 +52,15 @@ abstract class AbstractLspAnalysisServerTest DiscoveredPluginInfo configureTestPlugin({ plugin.ResponseResult respondWith, plugin.Notification notification, + Duration respondAfter = Duration.zero, }) { final info = DiscoveredPluginInfo('a', 'b', 'c', null, null); pluginManager.plugins.add(info); if (respondWith != null) { pluginManager.broadcastResults = >{ - info: Future.value(respondWith.toResponse('-', 1)) + info: Future.delayed(respondAfter) + .then((_) => respondWith.toResponse('-', 1)) }; } diff --git a/pkg/analysis_server/test/mocks.dart b/pkg/analysis_server/test/mocks.dart index ad4fa8cf9c5..11ad43c9368 100644 --- a/pkg/analysis_server/test/mocks.dart +++ b/pkg/analysis_server/test/mocks.dart @@ -39,6 +39,12 @@ class MockLspServerChannel implements LspServerCommunicationChannel { /// Completer that will be signalled when the input stream is closed. final Completer _closed = Completer(); + /// Errors popups sent to the user. + final shownErrors = []; + + /// Warning popups sent to the user. + final shownWarnings = []; + MockLspServerChannel(bool _printMessages) { if (_printMessages) { _serverToClient.stream @@ -46,6 +52,20 @@ class MockLspServerChannel implements LspServerCommunicationChannel { _clientToServer.stream .listen((message) => print('==> ' + jsonEncode(message))); } + + // Keep track of any errors/warnings that are sent to the user with + // `window/showMessage`. + _serverToClient.stream.listen((message) { + if (message is lsp.NotificationMessage && + message.method == Method.window_showMessage && + message.params is lsp.ShowMessageParams) { + if (message.params?.type == MessageType.Error) { + shownErrors.add(message.params); + } else if (message.params?.type == MessageType.Warning) { + shownWarnings.add(message.params); + } + } + }); } /// Future that will be completed when the input stream is closed. @@ -167,7 +187,8 @@ class MockLspServerChannel implements LspServerCommunicationChannel { (message is lsp.ResponseMessage && message.id == request.id) || (throwOnError && message is lsp.NotificationMessage && - message.method == Method.window_showMessage)); + message.method == Method.window_showMessage && + message.params?.type == MessageType.Error)); if (response is lsp.ResponseMessage) { return response;