[Analyzer] Cancel LSP refactor requests when another refactor request arrives
See https://github.com/dart-lang/sdk/issues/43459.
Change-Id: I52bf26e80b0d6ddb577c89b5a18ff7563e614cb4
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/165321
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Danny Tuppeny <danny@tuppeny.com>
diff --git a/pkg/analysis_server/lib/src/lsp/handlers/commands/organize_imports.dart b/pkg/analysis_server/lib/src/lsp/handlers/commands/organize_imports.dart
index 85f8c67..8fa665f 100644
--- a/pkg/analysis_server/lib/src/lsp/handlers/commands/organize_imports.dart
+++ b/pkg/analysis_server/lib/src/lsp/handlers/commands/organize_imports.dart
@@ -6,6 +6,7 @@
import 'package:analysis_server/lsp_protocol/protocol_special.dart';
import 'package:analysis_server/src/lsp/constants.dart';
import 'package:analysis_server/src/lsp/handlers/commands/simple_edit_handler.dart';
+import 'package:analysis_server/src/lsp/handlers/handlers.dart';
import 'package:analysis_server/src/lsp/lsp_analysis_server.dart';
import 'package:analysis_server/src/services/correction/organize_imports.dart';
@@ -16,7 +17,8 @@
String get commandName => 'Organize Imports';
@override
- Future<ErrorOr<void>> handle(List<dynamic> arguments) async {
+ Future<ErrorOr<void>> handle(
+ List<dynamic> arguments, CancellationToken cancellationToken) async {
if (arguments == null || arguments.length != 1 || arguments[0] is! String) {
return ErrorOr.error(ResponseError(
code: ServerErrorCodes.InvalidCommandArguments,
@@ -32,6 +34,11 @@
final docIdentifier = server.getVersionedDocumentIdentifier(path);
final result = await requireResolvedUnit(path);
+
+ if (cancellationToken.isCancellationRequested) {
+ return error(ErrorCodes.RequestCancelled, 'Request was cancelled');
+ }
+
return result.mapResult((result) {
final code = result.content;
final unit = result.unit;
diff --git a/pkg/analysis_server/lib/src/lsp/handlers/commands/perform_refactor.dart b/pkg/analysis_server/lib/src/lsp/handlers/commands/perform_refactor.dart
index c81db2e..714c7ad 100644
--- a/pkg/analysis_server/lib/src/lsp/handlers/commands/perform_refactor.dart
+++ b/pkg/analysis_server/lib/src/lsp/handlers/commands/perform_refactor.dart
@@ -6,12 +6,15 @@
import 'package:analysis_server/lsp_protocol/protocol_special.dart';
import 'package:analysis_server/src/lsp/constants.dart';
import 'package:analysis_server/src/lsp/handlers/commands/simple_edit_handler.dart';
+import 'package:analysis_server/src/lsp/handlers/handlers.dart';
import 'package:analysis_server/src/lsp/lsp_analysis_server.dart';
import 'package:analysis_server/src/lsp/mapping.dart';
import 'package:analysis_server/src/protocol_server.dart';
import 'package:analysis_server/src/services/refactoring/refactoring.dart';
import 'package:analyzer/dart/analysis/results.dart';
+final _manager = _RefactorManager();
+
class PerformRefactorCommandHandler extends SimpleEditCommandHandler {
PerformRefactorCommandHandler(LspAnalysisServer server) : super(server);
@@ -19,7 +22,8 @@
String get commandName => 'Perform Refactor';
@override
- Future<ErrorOr<void>> handle(List<dynamic> arguments) async {
+ Future<ErrorOr<void>> handle(
+ List<dynamic> arguments, CancellationToken cancellationToken) async {
if (arguments == null ||
arguments.length != 6 ||
arguments[0] is! String || // kind
@@ -49,24 +53,45 @@
return _getRefactoring(
RefactoringKind(kind), result, offset, length, options)
.mapResult((refactoring) async {
- final status = await refactoring.checkAllConditions();
-
- if (status.hasError) {
- return error(ServerErrorCodes.RefactorFailed, status.message);
+ // If the token we were given is not cancellable, replace it with one that
+ // is for the rest of this request, as a future refactor may need to cancel
+ // this request.
+ if (cancellationToken is! CancelableToken) {
+ cancellationToken = CancelableToken();
}
+ _manager.begin(cancellationToken);
- final change = await refactoring.createChange();
+ try {
+ final status = await refactoring.checkAllConditions();
- // If the file changed while we were validating and preparing the change,
- // we should fail to avoid sending bad edits.
- if (docVersion != null &&
- docVersion != server.getVersionedDocumentIdentifier(path).version) {
- return error(ErrorCodes.ContentModified,
- 'Content was modified before refactor was applied');
+ if (status.hasError) {
+ return error(ServerErrorCodes.RefactorFailed, status.message);
+ }
+
+ if (cancellationToken.isCancellationRequested) {
+ return error(ErrorCodes.RequestCancelled, 'Request was cancelled');
+ }
+
+ final change = await refactoring.createChange();
+
+ if (cancellationToken.isCancellationRequested) {
+ return error(ErrorCodes.RequestCancelled, 'Request was cancelled');
+ }
+
+ // If the file changed while we were validating and preparing the change,
+ // we should fail to avoid sending bad edits.
+ if (docVersion != null &&
+ docVersion !=
+ server.getVersionedDocumentIdentifier(path).version) {
+ return error(ErrorCodes.ContentModified,
+ 'Content was modified before refactor was applied');
+ }
+
+ final edit = createWorkspaceEdit(server, change.edits);
+ return await sendWorkspaceEditToClient(edit);
+ } finally {
+ _manager.end(cancellationToken);
}
-
- final edit = createWorkspaceEdit(server, change.edits);
- return await sendWorkspaceEditToClient(edit);
});
});
}
@@ -112,3 +137,22 @@
}
}
}
+
+/// Manages a running refactor to help ensure only one refactor runs at a time.
+class _RefactorManager {
+ /// The cancellation token for the current in-progress refactor (or null).
+ CancelableToken _currentRefactoringCancellationToken;
+
+ /// Begins a new refactor, cancelling any other in-progress refactors.
+ void begin(CancelableToken cancelToken) {
+ _currentRefactoringCancellationToken?.cancel();
+ _currentRefactoringCancellationToken = cancelToken;
+ }
+
+ /// Marks a refactor as no longer current.
+ void end(CancelableToken cancelToken) {
+ if (_currentRefactoringCancellationToken == cancelToken) {
+ _currentRefactoringCancellationToken = null;
+ }
+ }
+}
diff --git a/pkg/analysis_server/lib/src/lsp/handlers/commands/send_workspace_edit.dart b/pkg/analysis_server/lib/src/lsp/handlers/commands/send_workspace_edit.dart
index c918eb6..20a0f06 100644
--- a/pkg/analysis_server/lib/src/lsp/handlers/commands/send_workspace_edit.dart
+++ b/pkg/analysis_server/lib/src/lsp/handlers/commands/send_workspace_edit.dart
@@ -6,6 +6,7 @@
import 'package:analysis_server/lsp_protocol/protocol_special.dart';
import 'package:analysis_server/src/lsp/constants.dart';
import 'package:analysis_server/src/lsp/handlers/commands/simple_edit_handler.dart';
+import 'package:analysis_server/src/lsp/handlers/handlers.dart';
import 'package:analysis_server/src/lsp/lsp_analysis_server.dart';
/// This command allows a client to request the server send it a
@@ -22,7 +23,8 @@
String get commandName => 'Send Workspace Edit';
@override
- Future<ErrorOr<void>> handle(List<dynamic> arguments) async {
+ Future<ErrorOr<void>> handle(
+ List<dynamic> arguments, CancellationToken cancellationToken) async {
if (arguments == null ||
arguments.length != 1 ||
arguments[0] is! Map<String, dynamic>) {
diff --git a/pkg/analysis_server/lib/src/lsp/handlers/commands/sort_members.dart b/pkg/analysis_server/lib/src/lsp/handlers/commands/sort_members.dart
index c1a5b8f..304deba 100644
--- a/pkg/analysis_server/lib/src/lsp/handlers/commands/sort_members.dart
+++ b/pkg/analysis_server/lib/src/lsp/handlers/commands/sort_members.dart
@@ -6,6 +6,7 @@
import 'package:analysis_server/lsp_protocol/protocol_special.dart';
import 'package:analysis_server/src/lsp/constants.dart';
import 'package:analysis_server/src/lsp/handlers/commands/simple_edit_handler.dart';
+import 'package:analysis_server/src/lsp/handlers/handlers.dart';
import 'package:analysis_server/src/lsp/lsp_analysis_server.dart';
import 'package:analysis_server/src/services/correction/sort_members.dart';
@@ -16,7 +17,8 @@
String get commandName => 'Sort Members';
@override
- Future<ErrorOr<void>> handle(List<dynamic> arguments) async {
+ Future<ErrorOr<void>> handle(
+ List<dynamic> arguments, CancellationToken cancellationToken) async {
if (arguments == null || arguments.length != 1 || arguments[0] is! String) {
return ErrorOr.error(ResponseError(
code: ServerErrorCodes.InvalidCommandArguments,
@@ -33,6 +35,11 @@
var driver = server.getAnalysisDriver(path);
final result = await driver?.parseFile(path);
+
+ if (cancellationToken.isCancellationRequested) {
+ return error(ErrorCodes.RequestCancelled, 'Request was cancelled');
+ }
+
if (result == null) {
return ErrorOr.error(ResponseError(
code: ServerErrorCodes.FileNotAnalyzed,
diff --git a/pkg/analysis_server/lib/src/lsp/handlers/handler_execute_command.dart b/pkg/analysis_server/lib/src/lsp/handlers/handler_execute_command.dart
index 22b99d1..b966d78 100644
--- a/pkg/analysis_server/lib/src/lsp/handlers/handler_execute_command.dart
+++ b/pkg/analysis_server/lib/src/lsp/handlers/handler_execute_command.dart
@@ -41,6 +41,6 @@
return error(ServerErrorCodes.UnknownCommand,
'${params.command} is not a valid command identifier', null);
}
- return handler.handle(params.arguments);
+ return handler.handle(params.arguments, token);
}
}
diff --git a/pkg/analysis_server/lib/src/lsp/handlers/handlers.dart b/pkg/analysis_server/lib/src/lsp/handlers/handlers.dart
index c87b9c7..8e86fb0 100644
--- a/pkg/analysis_server/lib/src/lsp/handlers/handlers.dart
+++ b/pkg/analysis_server/lib/src/lsp/handlers/handlers.dart
@@ -47,7 +47,8 @@
this.server = server;
}
- Future<ErrorOr<void>> handle(List<dynamic> arguments);
+ Future<ErrorOr<void>> handle(
+ List<dynamic> arguments, CancellationToken cancellationToken);
}
mixin Handler<P, R> {
diff --git a/pkg/analysis_server/test/lsp/code_actions_refactor_test.dart b/pkg/analysis_server/test/lsp/code_actions_refactor_test.dart
index 2701b41..19589b0 100644
--- a/pkg/analysis_server/test/lsp/code_actions_refactor_test.dart
+++ b/pkg/analysis_server/test/lsp/code_actions_refactor_test.dart
@@ -2,11 +2,13 @@
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
+import 'package:analysis_server/lsp_protocol/protocol_generated.dart';
import 'package:analysis_server/src/lsp/constants.dart';
import 'package:test/test.dart';
import 'package:test_reflective_loader/test_reflective_loader.dart';
import '../src/utilities/mock_packages.dart';
+import '../tool/lsp_spec/matchers.dart';
import 'code_actions_abstract.dart';
void main() {
@@ -49,6 +51,60 @@
codeAction, withoutMarkers(content), expectedContent);
}
+ Future<void> test_cancelsInProgress() async {
+ const content = '''
+main() {
+ print('Test!');
+ [[print('Test!');]]
+}
+ ''';
+ const expectedContent = '''
+main() {
+ print('Test!');
+ newMethod();
+}
+
+void newMethod() {
+ print('Test!');
+}
+ ''';
+ newFile(mainFilePath, content: withoutMarkers(content));
+ await initialize();
+
+ final codeActions = await getCodeActions(mainFileUri.toString(),
+ range: rangeFromMarkers(content));
+ final codeAction =
+ findCommand(codeActions, Commands.performRefactor, extractMethodTitle);
+ expect(codeAction, isNotNull);
+
+ // Respond to any applyEdit requests from the server with successful responses
+ // and capturing the last edit.
+ WorkspaceEdit edit;
+ requestsFromServer.listen((request) async {
+ if (request.method == Method.workspace_applyEdit) {
+ final params = ApplyWorkspaceEditParams.fromJson(request.params);
+ edit = params.edit;
+ respondTo(request, ApplyWorkspaceEditResponse(applied: true));
+ }
+ });
+
+ // Send two requests together.
+ final req1 = executeCodeAction(codeAction);
+ final req2 = executeCodeAction(codeAction);
+
+ // Expect the first will have cancelled the second.
+ await expectLater(
+ req1, throwsA(isResponseError(ErrorCodes.RequestCancelled)));
+ await req2;
+
+ // Ensure applying the changes will give us the expected content.
+ final contents = {
+ mainFilePath: withoutMarkers(content),
+ };
+ applyChanges(contents, edit.changes);
+ expect(contents[mainFilePath], equals(expectedContent));
+ }
+
Future<void> test_invalidLocation() async {
const content = '''
import 'dart:convert';
diff --git a/pkg/analysis_server/test/lsp/server_abstract.dart b/pkg/analysis_server/test/lsp/server_abstract.dart
index 5029f4c..b62837b 100644
--- a/pkg/analysis_server/test/lsp/server_abstract.dart
+++ b/pkg/analysis_server/test/lsp/server_abstract.dart
@@ -585,6 +585,15 @@
await sendNotificationToServer(notification);
}
+ Future<Object> executeCodeAction(
+ Either2<Command, CodeAction> codeAction) async {
+ final command = codeAction.map(
+ (command) => command,
+ (codeAction) => codeAction.command,
+ );
+ return executeCommand(command);
+ }
+
Future<Object> executeCommand(Command command) async {
final request = makeRequest(
Method.workspace_executeCommand,
@@ -939,13 +948,20 @@
@required FutureOr<RR> Function(R) handler,
Duration timeout = const Duration(seconds: 5),
}) async {
- FutureOr<T> outboundRequest;
+ Future<T> outboundRequest;
// Run [f] and wait for the incoming request from the server.
final incomingRequest = await expectRequest(method, () {
// Don't return/await the response yet, as this may not complete until
// after we have handled the request that comes from the server.
outboundRequest = f();
+
+ // Because we don't await this future until "later", if it throws the
+ // error is treated as unhandled and will fail the test. Attaching an
+ // error handler prevents that, though since the Future completed with
+ // an error it will still be handled as such when the future is later
+ // awaited.
+ outboundRequest.catchError((_) {});
});
// Handle the request from the server and send the response back.
diff --git a/pkg/analysis_server/test/mocks.dart b/pkg/analysis_server/test/mocks.dart
index 396a19f..c166c2c 100644
--- a/pkg/analysis_server/test/mocks.dart
+++ b/pkg/analysis_server/test/mocks.dart
@@ -186,7 +186,8 @@
(throwOnError &&
message is lsp.NotificationMessage &&
message.method == Method.window_showMessage &&
- message.params?.type == MessageType.Error));
+ lsp.ShowMessageParams.fromJson(message.params).type ==
+ MessageType.Error));
if (response is lsp.ResponseMessage) {
return response;
