Don't remove overlays immediately, wait for the next watch event.

Change-Id: Ie3352bf70010de88874aea122d83a5533a00a1ca
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/235740
Reviewed-by: Brian Wilkerson <[email protected]>
Reviewed-by: Jaime Wren <[email protected]>
Commit-Queue: Konstantin Shcheglov <[email protected]>

Author: scheglov

pkg/analysis_server/lib/src/analysis_server.dart

@@ -47,6 +47,7 @@ import 'package:analyzer/dart/analysis/results.dart';
 import 'package:analyzer/dart/ast/ast.dart';
 import 'package:analyzer/exception/exception.dart';
 import 'package:analyzer/file_system/file_system.dart';
+import 'package:analyzer/file_system/overlay_file_system.dart';
 import 'package:analyzer/instrumentation/instrumentation.dart';
 import 'package:analyzer/src/dart/analysis/driver.dart' as analysis;
 import 'package:analyzer/src/dart/analysis/status.dart' as analysis;
@@ -123,6 +124,34 @@ class AnalysisServer extends AbstractAnalysisServer {
   final StreamController _onAnalysisSetChangedController =
       StreamController.broadcast(sync: true);
 
+  /// Key: a file path for which removing of the overlay was requested.
+  /// Value: a timer that will remove the overlay, or cancelled.
+  ///
+  /// This helps for analysis server running remotely, with slow remote file
+  /// systems, in the following scenario:
+  /// 1. User edits file, IDE sends "add overlay".
+  /// 2. User saves file, IDE saves file locally, sends "remove overlay".
+  /// 3. The remove server reads the file on "remove overlay". But the content
+  ///    of the file on the remove machine is not the same as it is locally,
+  ///    and not what the user looks in the IDE. So, analysis results, such
+  ///    as semantic highlighting, are inconsistent with the local file content.
+  /// 4. (after a few seconds) The file is synced to the remove machine,
+  ///    the watch event happens, server reads the file, sends analysis
+  ///    results that are consistent with the local file content.
+  ///
+  /// We try to prevent the inconsistency between moments (3) and (4).
+  /// It is not wrong, we are still in the eventual consistency, but we
+  /// want to keep the inconsistency time shorter.
+  ///
+  /// To do this we keep the last overlay content on "remove overlay",
+  /// and wait for the next watch event in (4). But there might be race
+  /// condition, and when it happens, we still want to get to the eventual
+  /// consistency, so on timer we remove the overlay anyway.
+  final Map<String, Timer> _pendingFilesToRemoveOverlay = {};
+
+  @visibleForTesting
+  Duration pendingFilesRemoveOverlayDelay = const Duration(seconds: 10);
+
   final DetachableFileSystemManager? detachableFileSystemManager;
 
   /// The broadcast stream of requests that were discarded because there
@@ -231,6 +260,12 @@ class AnalysisServer extends AbstractAnalysisServer {
     cancellationTokens[id]?.cancel();
   }
 
+  Future<void> dispose() async {
+    for (var timer in _pendingFilesToRemoveOverlay.values) {
+      timer.cancel();
+    }
+  }
+
   /// The socket from which requests are being read has been closed.
   void done() {}
 
@@ -511,7 +546,7 @@ class AnalysisServer extends AbstractAnalysisServer {
       } catch (_) {}
 
       // Prepare the new contents.
-      String? newContents;
+      String newContents;
       if (change is AddContentOverlay) {
         newContents = change.content;
       } else if (change is ChangeContentOverlay) {
@@ -530,25 +565,28 @@ class AnalysisServer extends AbstractAnalysisServer {
                   'Invalid overlay change')));
         }
       } else if (change is RemoveContentOverlay) {
-        newContents = null;
+        _pendingFilesToRemoveOverlay[file]?.cancel();
+        _pendingFilesToRemoveOverlay[file] = Timer(
+          pendingFilesRemoveOverlayDelay,
+          () {
+            resourceProvider.removeOverlay(file);
+            _changeFileInDrivers(file);
+          },
+        );
+        return;
       } else {
         // Protocol parsing should have ensured that we never get here.
         throw AnalysisException('Illegal change type');
       }
 
-      if (newContents != null) {
-        resourceProvider.setOverlay(
-          file,
-          content: newContents,
-          modificationStamp: overlayModificationStamp++,
-        );
-      } else {
-        resourceProvider.removeOverlay(file);
-      }
+      _pendingFilesToRemoveOverlay[file]?.cancel();
+      resourceProvider.setOverlay(
+        file,
+        content: newContents,
+        modificationStamp: overlayModificationStamp++,
+      );
 
-      driverMap.values.forEach((driver) {
-        driver.changeFile(file);
-      });
+      _changeFileInDrivers(file);
 
       // If the file did not exist, and is "overlay only", it still should be
       // analyzed. Add it to driver to which it should have been added.
@@ -586,6 +624,12 @@ class AnalysisServer extends AbstractAnalysisServer {
 //    });
   }
 
+  void _changeFileInDrivers(String path) {
+    for (var driver in driverMap.values) {
+      driver.changeFile(path);
+    }
+  }
+
   /// Returns `true` if there is a subscription for the given [service] and
   /// [file].
   bool _hasAnalysisServiceSubscription(AnalysisService service, String file) {
@@ -671,7 +715,7 @@ class ServerContextManagerCallbacks extends ContextManagerCallbacks {
   final AnalysisServer analysisServer;
 
   /// The [ResourceProvider] by which paths are converted into [Resource]s.
-  final ResourceProvider resourceProvider;
+  final OverlayResourceProvider resourceProvider;
 
   /// The set of files for which notifications were sent.
   final Set<String> filesToFlush = {};
@@ -698,6 +742,15 @@ class ServerContextManagerCallbacks extends ContextManagerCallbacks {
 
   @override
   void afterWatchEvent(WatchEvent event) {
+    var path = event.path;
+
+    var pendingTimer = analysisServer._pendingFilesToRemoveOverlay.remove(path);
+    if (pendingTimer != null) {
+      pendingTimer.cancel();
+      resourceProvider.removeOverlay(path);
+      analysisServer._changeFileInDrivers(path);
+    }
+
     analysisServer._onAnalysisSetChangedController.add(null);
   }
 

pkg/analysis_server/test/analysis/notification_errors_test.dart

@@ -45,6 +45,7 @@ class NotificationErrorsTest extends AbstractAnalysisTest {
   void setUp() {
     registerLintRules();
     super.setUp();
+    server.pendingFilesRemoveOverlayDelay = const Duration(milliseconds: 10);
     server.handlers = [
       AnalysisDomainHandler(server),
     ];
@@ -408,6 +409,10 @@ main() {
         brokenFile: RemoveContentOverlay(),
       }).toRequest('1'),
     );
+
+    // Wait for the timer to remove the overlay to fire.
+    await Future.delayed(server.pendingFilesRemoveOverlayDelay);
+
     await waitForTasksFinished();
     await pumpEventQueue(times: 5000);
 

pkg/analysis_server/test/domain_analysis_test.dart

@@ -569,8 +569,8 @@ transforms: []
 
     await setRoots(included: [workspaceRootPath], excluded: []);
 
-    // The test file is analyzed, no errors.
-    await server.onAnalysisComplete;
+    // The test file (overlay) is analyzed, no errors.
+    await _waitAnalysisComplete();
     _assertAnalyzedFiles(
       hasErrors: [],
       noErrors: [testFilePathPlatform],
@@ -581,21 +581,84 @@ transforms: []
     newFile(testFilePath, content: 'error');
 
     // But the overlay is still present, so the file is not analyzed.
-    await server.onAnalysisComplete;
+    await _waitAnalysisComplete();
     _assertAnalyzedFiles(
       hasErrors: [],
       notAnalyzed: [testFilePathPlatform],
     );
 
-    // Remove the overlay, now the file will be read.
+    // Ask to remove the overlay, still active, start a timer.
     await handleSuccessfulRequest(
       AnalysisUpdateContentParams({
         testFilePathPlatform: RemoveContentOverlay(),
       }).toRequest('0'),
     );
 
+    // Wait for the timer to remove the overlay to fire.
+    await Future.delayed(server.pendingFilesRemoveOverlayDelay);
+
     // The file has errors.
-    await server.onAnalysisComplete;
+    await _waitAnalysisComplete();
+    _assertAnalyzedFiles(
+      hasErrors: [testFilePathPlatform],
+      noErrors: [],
+      notAnalyzed: [],
+    );
+  }
+
+  Future<void>
+      test_fileSystem_changeFile_hasOverlay_removeOverlay_delayed() async {
+    // Use long delay, so that it does not happen.
+    server.pendingFilesRemoveOverlayDelay = const Duration(seconds: 300);
+
+    newFile(testFilePath, content: '');
+
+    // Add an overlay without errors.
+    await handleSuccessfulRequest(
+      AnalysisUpdateContentParams({
+        testFilePathPlatform: AddContentOverlay(''),
+      }).toRequest('0'),
+    );
+
+    await setRoots(included: [workspaceRootPath], excluded: []);
+
+    // The test file (overlay) is analyzed, no errors.
+    await _waitAnalysisComplete();
+    _assertAnalyzedFiles(
+      hasErrors: [],
+      noErrors: [testFilePathPlatform],
+      notAnalyzed: [],
+    );
+
+    // Change the file, has errors.
+    modifyFile(testFilePath, 'error');
+
+    // But the overlay is still present, so the file is not analyzed.
+    await _waitAnalysisComplete();
+    _assertAnalyzedFiles(
+      hasErrors: [],
+      notAnalyzed: [testFilePathPlatform],
+    );
+
+    // Ask to remove the overlay, still active, start a timer.
+    await handleSuccessfulRequest(
+      AnalysisUpdateContentParams({
+        testFilePathPlatform: RemoveContentOverlay(),
+      }).toRequest('0'),
+    );
+
+    // Long timer, so still not analyzed.
+    await _waitAnalysisComplete();
+    _assertAnalyzedFiles(
+      hasErrors: [],
+      notAnalyzed: [testFilePathPlatform],
+    );
+
+    // Change the file again, has errors.
+    newFile(testFilePath, content: 'error');
+
+    // The timer cancelled on the watch event, and the file analyzed.
+    await _waitAnalysisComplete();
     _assertAnalyzedFiles(
       hasErrors: [testFilePathPlatform],
       noErrors: [],
@@ -1280,6 +1343,16 @@ void f(A a) {}
       ),
     );
   }
+
+  /// Pump the event queue, so that watch events are processed.
+  /// Wait for analysis to complete.
+  /// Repeat a few times, eventually there will be no work to do.
+  Future<void> _waitAnalysisComplete() async {
+    for (var i = 0; i < 128; i++) {
+      pumpEventQueue();
+      await server.onAnalysisComplete;
+    }
+  }
 }
 
 @reflectiveTest

pkg/analysis_server/test/domain_completion_test.dart

@@ -3016,9 +3016,14 @@ class PubPackageAnalysisServerTest with ResourceProviderMixin {
       InstrumentationService.NULL_SERVICE,
     );
 
+    server.pendingFilesRemoveOverlayDelay = const Duration(milliseconds: 10);
     completionDomain.budgetDuration = const Duration(seconds: 30);
   }
 
+  Future<void> tearDown() async {
+    await server.dispose();
+  }
+
   void writePackageConfig(Folder root, PackageConfigFileBuilder config) {
     newPackageConfigJsonFile(
       root.path,