[analysis_server] Cancel in-progress computation of fixes if another check starts

Change-Id: I3d2a740fbc077ba1707837bb9f2701de70da6ed7
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/290520
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>

pkg/analysis_server/lib/src/lsp/handlers/commands/fix_all.dart

@@ -43,8 +43,9 @@ class FixAllCommandHandler extends SimpleEditCommandHandler {
       final workspace = DartChangeWorkspace(
         await server.currentSessions,
       );
-      final processor =
-          BulkFixProcessor(server.instrumentationService, workspace);
+      final processor = BulkFixProcessor(
+          server.instrumentationService, workspace,
+          cancellationToken: cancellationToken);
 
       final context = server.contextManager.getContextFor(path);
       if (context == null) {

pkg/analysis_server/lib/src/services/correction/bulk_fix_processor.dart

@@ -25,6 +25,7 @@ import 'package:analyzer/source/error_processor.dart';
 import 'package:analyzer/src/error/codes.dart';
 import 'package:analyzer/src/lint/registry.dart';
 import 'package:analyzer/src/util/file_paths.dart' as file_paths;
+import 'package:analyzer/src/utilities/cancellation.dart';
 import 'package:analyzer/src/utilities/extensions/string.dart';
 import 'package:analyzer_plugin/src/utilities/change_builder/change_builder_core.dart';
 import 'package:analyzer_plugin/utilities/change_builder/change_builder_core.dart';
@@ -172,11 +173,20 @@ class BulkFixProcessor {
   /// A map associating libraries to fixes with change counts.
   final ChangeMap changeMap = ChangeMap();
 
+  /// A token used to signal that the caller is no longer interested in the
+  /// results and processing can end early (in which case any results may be
+  /// invalid).
+  final CancellationToken? cancellationToken;
+
   /// Initialize a newly created processor to create fixes for diagnostics in
   /// libraries in the [workspace].
-  BulkFixProcessor(this.instrumentationService, this.workspace,
-      {this.useConfigFiles = false, List<String>? codes})
-      : builder = ChangeBuilder(workspace: workspace),
+  BulkFixProcessor(
+    this.instrumentationService,
+    this.workspace, {
+    this.useConfigFiles = false,
+    List<String>? codes,
+    this.cancellationToken,
+  })  : builder = ChangeBuilder(workspace: workspace),
         codes = codes?.map((e) => e.toLowerCase()).toList();
 
   List<BulkFix> get fixDetails {
@@ -191,6 +201,8 @@ class BulkFixProcessor {
     return details;
   }
 
+  bool get isCancelled => cancellationToken?.isCancellationRequested ?? false;
+
   /// Return a [BulkFixRequestResult] that includes a change builder that has
   /// been used to create fixes for the diagnostics in the libraries in the
   /// given [contexts].
@@ -205,7 +217,7 @@ class BulkFixProcessor {
 
     if (file_paths.isDart(pathContext, path) && !file_paths.isGenerated(path)) {
       var library = await context.currentSession.getResolvedLibrary(path);
-      if (library is ResolvedLibraryResult) {
+      if (!isCancelled && library is ResolvedLibraryResult) {
         await _fixErrorsInLibrary(library);
       }
     }
@@ -276,9 +288,12 @@ class BulkFixProcessor {
         }
 
         var library = await context.currentSession.getResolvedLibrary(path);
+        if (isCancelled) {
+          break;
+        }
         if (library is ResolvedLibraryResult) {
           await _fixErrorsInLibrary(library, stopAfterFirst: stopAfterFirst);
-          if (stopAfterFirst && changeMap.hasFixes) {
+          if (isCancelled || (stopAfterFirst && changeMap.hasFixes)) {
             break;
           }
         }
@@ -345,7 +360,7 @@ class BulkFixProcessor {
       for (var error in filteredErrors(unitResult)) {
         await _fixSingleError(
             fixContext(unitResult, error), unitResult, error, overrideSet);
-        if (stopAfterFirst && changeMap.hasFixes) {
+        if (isCancelled || (stopAfterFirst && changeMap.hasFixes)) {
           return;
         }
       }
@@ -379,7 +394,7 @@ class BulkFixProcessor {
         if (context != null) {
           await _generateFix(context, OrganizeImports(),
               directivesOrderingError.errorCode.name);
-          if (stopAfterFirst && changeMap.hasFixes) {
+          if (isCancelled || (stopAfterFirst && changeMap.hasFixes)) {
             return;
           }
         }
@@ -389,7 +404,7 @@ class BulkFixProcessor {
           if (context != null) {
             await _generateFix(
                 context, RemoveUnusedImport(), error.errorCode.name);
-            if (stopAfterFirst && changeMap.hasFixes) {
+            if (isCancelled || (stopAfterFirst && changeMap.hasFixes)) {
               return;
             }
           }
@@ -426,6 +441,9 @@ class BulkFixProcessor {
         var producer = generator();
         if (producer.canBeAppliedInBulk) {
           await _generateFix(context, producer, codeName);
+          if (isCancelled) {
+            return;
+          }
         }
       }
     }
@@ -436,9 +454,15 @@ class BulkFixProcessor {
       if (errorCode is LintCode) {
         var generators = FixProcessor.lintProducerMap[codeName] ?? [];
         await bulkApply(generators, codeName);
+        if (isCancelled) {
+          return;
+        }
       } else {
         var generators = FixProcessor.nonLintProducerMap[errorCode] ?? [];
         await bulkApply(generators, codeName);
+        if (isCancelled) {
+          return;
+        }
         var multiGenerators = nonLintMultiProducerMap[errorCode];
         if (multiGenerators != null) {
           for (var multiGenerator in multiGenerators) {
@@ -446,6 +470,9 @@ class BulkFixProcessor {
             multiProducer.configure(context);
             for (var producer in await multiProducer.producers) {
               await _generateFix(context, producer, codeName);
+              if (isCancelled) {
+                return;
+              }
             }
           }
         }

pkg/analysis_server/lib/src/services/user_prompts/dart_fix_prompt_manager.dart

@@ -5,6 +5,7 @@
 import 'dart:async';
 
 import 'package:analysis_server/lsp_protocol/protocol.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/bulk_fix_processor.dart';
 import 'package:analysis_server/src/services/correction/change_workspace.dart';
@@ -58,17 +59,10 @@ class DartFixPromptManager {
   /// context paths) additional checks are allowed.
   Map<String, String?> _lastContextSdkVersionConstraints = {};
 
+  CancelableToken? _inProgressCheckCancellationToken;
+
   DartFixPromptManager(this.server, this.preferences);
 
-  @visibleForTesting
-  Future<bool> get bulkFixesAvailable async {
-    final workspace = DartChangeWorkspace(await server.currentSessions);
-    final processor =
-        BulkFixProcessor(server.instrumentationService, workspace);
-
-    return processor.hasFixes(server.contextManager.analysisContexts);
-  }
-
   /// Gets a map of context root paths to their version constraint strings.
   @visibleForTesting
   Map<String, String?> get currentContextSdkConstraints {
@@ -85,6 +79,41 @@ class DartFixPromptManager {
         DateTime.now().difference(lastCheck) <= _sleepTime;
   }
 
+  /// Whether or not "dart fix" may be able to fix diagnostics in the project.
+  ///
+  /// This method is exposed to allow tests to override the results. It should
+  /// only be called by [performCheck]. Other callers interested in the results
+  /// should call [performCheck] which handles cancelling other in-progress
+  /// checks.
+  @visibleForTesting
+  Future<bool> bulkFixesAvailable(CancellationToken token) async {
+    final sessions = await server.currentSessions;
+    if (token.isCancellationRequested) {
+      return false;
+    }
+
+    final workspace = DartChangeWorkspace(sessions);
+    final processor = BulkFixProcessor(server.instrumentationService, workspace,
+        cancellationToken: token);
+
+    return processor.hasFixes(server.contextManager.analysisContexts);
+  }
+
+  /// Performs a check for bulk fixes, cancelling any other in-progress checks.
+  Future<bool> performCheck() async {
+    // Signal that any in-progress check should abort.
+    _inProgressCheckCancellationToken?.cancel();
+
+    // Assign a new token for this check.
+    final token = _inProgressCheckCancellationToken = CancelableToken();
+    final fixesAvailable = await bulkFixesAvailable(token);
+
+    // If we were cancelled since the last cancellation check inside
+    // bulkFixesAvailable, still return false because another check is now in
+    // progress and our results are stale.
+    return fixesAvailable && !token.isCancellationRequested;
+  }
+
   @visibleForTesting
   Future<void> showPrompt() async {
     _hasPromptedThisSession = true;
@@ -121,7 +150,7 @@ class DartFixPromptManager {
   /// context rebuild).
   void triggerCheck() {
     unawaited(
-      _performCheck().catchError((e) {
+      _performCheckAndPrompt().catchError((e) {
         server.instrumentationService
             .logError('Failed to perform bulk "dart fix" check: $e');
       }),
@@ -132,7 +161,12 @@ class DartFixPromptManager {
     server.sendOpenUriNotification(learnMoreUri);
   }
 
-  Future<void> _performCheck() async {
+  /// Performs a check to see if "dart fix" may be able to fix diagnostics in
+  /// the project and if so, prompts the user.
+  ///
+  /// The check/prompt may be skipped if not supported or the check has been run
+  /// recently. If an existing check is in-progress, it will be aborted.
+  Future<void> _performCheckAndPrompt() async {
     if (_hasPromptedThisSession ||
         !server.supportsShowMessageRequest ||
         !server.supportsOpenUriNotification ||
@@ -152,7 +186,7 @@ class DartFixPromptManager {
 
     // Perform the (potentially expensive) check.
     lastCheck = DateTime.now();
-    if (!(await bulkFixesAvailable)) {
+    if (!(await performCheck())) {
       return;
     }
 

pkg/analysis_server/test/services/user_prompts/dart_fix_prompt_manager_test.dart

@@ -2,6 +2,7 @@
 // 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/src/lsp/handlers/handlers.dart';
 import 'package:analysis_server/src/lsp/lsp_analysis_server.dart';
 import 'package:analysis_server/src/services/user_prompts/dart_fix_prompt_manager.dart';
 import 'package:analysis_server/src/services/user_prompts/user_prompts.dart';
@@ -135,6 +136,18 @@ class DartFixPromptTest with ResourceProviderMixin {
     expect(promptManager.checksPerformed, 1);
   }
 
+  Future<void> test_check_returnsFalseIfCancelled() async {
+    // Trigger 50 checks at once. Each one should cancel the previous, with only
+    // the final one completing.
+    final futures = List.generate(50, (_) => promptManager.performCheck());
+    await pumpEventQueue(times: 5000);
+    final results = await Future.wait(futures);
+
+    // Expect the first 49 to be false, the last to be true.
+    expect(results.sublist(0, 49), everyElement(isFalse));
+    expect(results.last, isTrue);
+  }
+
   Future<void> test_prompt_ifFixes() async {
     promptManager.triggerCheck();
     await pumpEventQueue(times: 5000);
@@ -182,7 +195,7 @@ class TestDartFixPromptManager extends DartFixPromptManager {
   TestDartFixPromptManager(super.server, super.preferences);
 
   @override
-  Future<bool> get bulkFixesAvailable {
+  Future<bool> bulkFixesAvailable(CancellationToken token) {
     checksPerformed++;
     return bulkFixesAvailableOverride;
   }

pkg/analysis_server/test/src/services/correction/fix/bulk_fix_processor_test.dart

@@ -2,10 +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/src/lsp/handlers/handlers.dart';
+import 'package:analysis_server/src/services/correction/bulk_fix_processor.dart';
 import 'package:analysis_server/src/services/linter/lint_names.dart';
 import 'package:test/test.dart';
 import 'package:test_reflective_loader/test_reflective_loader.dart';
 
+import '../../../../utils/test_instrumentation_service.dart';
 import 'fix_processor.dart';
 
 void main() {
@@ -37,6 +40,34 @@ var aa = new A();
     expect(errors, hasLength(1));
     expect(errors[LintNames.unnecessary_new], 2);
   }
+
+  Future<void> test_changeMap_cancelled() async {
+    createAnalysisOptionsFile(experiments: experiments, lints: [
+      LintNames.unnecessary_new,
+    ]);
+
+    await resolveTestCode('''
+class A { }
+
+var a = new A();
+''');
+
+    var analysisContext = contextFor(testFile);
+    var changeWorkspace = await workspace;
+    var token = CancelableToken();
+    var processor = BulkFixProcessor(
+        TestInstrumentationService(), changeWorkspace,
+        cancellationToken: token);
+
+    // Begin computing fixes, then immediately cancel.
+    var fixErrorsFuture = processor.fixErrors([analysisContext]);
+    token.cancel();
+
+    // Wait for code to return and expect that we didn't compute any changes
+    // (because we exited early).
+    await fixErrorsFuture;
+    expect(processor.changeMap.libraryMap, isEmpty);
+  }
 }
 
 @reflectiveTest