[beta] Remove delay in removing file overlays.

Issue 50960. Revert delaying removing file overlays.

This reverts 6de7ea7

This hurts our external users, and I got an impression that the
scenario for which we designed it initially is not the way our
internal developers works now.

Bug: #50960
Change-Id: I9b1ecf4860146d67963c5c99ecefc581e1526a89
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/278883
Commit-Queue: Konstantin Shcheglov <[email protected]>
Reviewed-by: Samuel Rawlins <[email protected]>
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/278993
Reviewed-by: Siva Annamalai <[email protected]>
Commit-Queue: Samuel Rawlins <[email protected]>
Reviewed-by: Konstantin Shcheglov <[email protected]>

Author: srawlins

pkg/analysis_server/lib/src/legacy_analysis_server.dart

@@ -319,34 +319,6 @@ class LegacyAnalysisServer extends AnalysisServer {
   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);
-
   /// An optional manager to handle file systems which may not always be
   /// available.
   final DetachableFileSystemManager? detachableFileSystemManager;
@@ -458,11 +430,7 @@ class LegacyAnalysisServer extends AnalysisServer {
     cancellationTokens[id]?.cancel();
   }
 
-  Future<void> dispose() async {
-    for (var timer in _pendingFilesToRemoveOverlay.values) {
-      timer.cancel();
-    }
-  }
+  Future<void> dispose() async {}
 
   /// The socket from which requests are being read has been closed.
   void done() {}
@@ -747,7 +715,7 @@ class LegacyAnalysisServer extends AnalysisServer {
       } catch (_) {}
 
       // Prepare the new contents.
-      String newContents;
+      String? newContents;
       if (change is AddContentOverlay) {
         newContents = change.content;
       } else if (change is ChangeContentOverlay) {
@@ -766,29 +734,25 @@ class LegacyAnalysisServer extends AnalysisServer {
                   'Invalid overlay change')));
         }
       } else if (change is RemoveContentOverlay) {
-        _pendingFilesToRemoveOverlay.remove(file)?.cancel();
-        _pendingFilesToRemoveOverlay[file] = Timer(
-          pendingFilesRemoveOverlayDelay,
-          () {
-            _pendingFilesToRemoveOverlay.remove(file);
-            resourceProvider.removeOverlay(file);
-            _changeFileInDrivers(file);
-          },
-        );
-        return;
+        newContents = null;
       } else {
         // Protocol parsing should have ensured that we never get here.
         throw AnalysisException('Illegal change type');
       }
 
-      _pendingFilesToRemoveOverlay.remove(file)?.cancel();
-      resourceProvider.setOverlay(
-        file,
-        content: newContents,
-        modificationStamp: overlayModificationStamp++,
-      );
+      if (newContents != null) {
+        resourceProvider.setOverlay(
+          file,
+          content: newContents,
+          modificationStamp: overlayModificationStamp++,
+        );
+      } else {
+        resourceProvider.removeOverlay(file);
+      }
 
-      _changeFileInDrivers(file);
+      for (var driver in driverMap.values) {
+        driver.changeFile(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.
@@ -826,12 +790,6 @@ class LegacyAnalysisServer extends AnalysisServer {
 //    });
   }
 
-  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) {
@@ -911,15 +869,6 @@ 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,7 +45,6 @@ class NotificationErrorsTest extends PubPackageAnalysisServerTest {
   void setUp() {
     registerLintRules();
     super.setUp();
-    server.pendingFilesRemoveOverlayDelay = const Duration(milliseconds: 10);
     pedanticFolder = MockPackages.instance.addPedantic(resourceProvider);
   }
 
@@ -407,9 +406,6 @@ void f() {
       }).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/analysis_server_base.dart

@@ -183,7 +183,6 @@ class ContextResolutionTest with ResourceProviderMixin {
       InstrumentationService.NULL_SERVICE,
     );
 
-    server.pendingFilesRemoveOverlayDelay = const Duration(milliseconds: 10);
     server.pluginManager = pluginManager;
     server.completionState.budgetDuration = const Duration(seconds: 30);
   }

pkg/analysis_server/test/domain_analysis_test.dart

@@ -588,16 +588,13 @@ transforms: []
       notAnalyzed: [testFile.path],
     );
 
-    // Ask to remove the overlay, still active, start a timer.
+    // Remove the overlay, now the file will be read.
     await handleSuccessfulRequest(
       AnalysisUpdateContentParams({
         testFile.path: RemoveContentOverlay(),
       }).toRequest('0'),
     );
 
-    // Wait for the timer to remove the overlay to fire.
-    await Future.delayed(server.pendingFilesRemoveOverlayDelay);
-
     // The file has errors.
     await _waitAnalysisComplete();
     _assertAnalyzedFiles(
@@ -607,66 +604,6 @@ transforms: []
     );
   }
 
-  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, '');
-
-    // Add an overlay without errors.
-    await handleSuccessfulRequest(
-      AnalysisUpdateContentParams({
-        testFile.path: AddContentOverlay(''),
-      }).toRequest('0'),
-    );
-
-    await setRoots(included: [workspaceRootPath], excluded: []);
-
-    // The test file (overlay) is analyzed, no errors.
-    await _waitAnalysisComplete();
-    _assertAnalyzedFiles(
-      hasErrors: [],
-      noErrors: [testFile.path],
-      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: [testFile.path],
-    );
-
-    // Ask to remove the overlay, still active, start a timer.
-    await handleSuccessfulRequest(
-      AnalysisUpdateContentParams({
-        testFile.path: RemoveContentOverlay(),
-      }).toRequest('0'),
-    );
-
-    // Long timer, so still not analyzed.
-    await _waitAnalysisComplete();
-    _assertAnalyzedFiles(
-      hasErrors: [],
-      notAnalyzed: [testFile.path],
-    );
-
-    // Change the file again, has errors.
-    newFile(testFilePath, 'error');
-
-    // The timer cancelled on the watch event, and the file analyzed.
-    await _waitAnalysisComplete();
-    _assertAnalyzedFiles(
-      hasErrors: [testFile.path],
-      noErrors: [],
-      notAnalyzed: [],
-    );
-  }
-
   Future<void> test_fileSystem_changeFile_packageConfigJsonFile() async {
     var aaaRootPath = '/packages/aaa';
     var a_path = '$aaaRootPath/lib/a.dart';
@@ -885,62 +822,6 @@ void f(A a) {}
     assertNoErrorsNotification(a_path);
   }
 
-  /// This test ensures that when an `addOverlay` cancels any pending
-  /// `removeOverlay` timer, it also removes it, so that a subsequent watch
-  /// event does not still try to process it.
-  Future<void>
-      test_fileSystem_removeOverlay_addOverlay_changeFile_changeOverlay() async {
-    // Use long delay, so that it does not happen.
-    server.pendingFilesRemoveOverlayDelay = const Duration(seconds: 300);
-
-    newFile(testFilePath, '');
-
-    // Add an overlay without errors.
-    await handleSuccessfulRequest(
-      AnalysisUpdateContentParams({
-        testFile.path: AddContentOverlay(''),
-      }).toRequest('0'),
-    );
-
-    await setRoots(included: [workspaceRootPath], excluded: []);
-
-    // The test file (overlay) is analyzed, no errors.
-    await _waitAnalysisComplete();
-    _assertAnalyzedFiles(
-      hasErrors: [],
-      noErrors: [testFile.path],
-      notAnalyzed: [],
-    );
-
-    // Ask to remove the overlay, still active, start a timer.
-    await handleSuccessfulRequest(
-      AnalysisUpdateContentParams({
-        testFile.path: RemoveContentOverlay(),
-      }).toRequest('0'),
-    );
-
-    // Re-add an overlay. Should cancel the timer and replace the overlay.
-    await handleSuccessfulRequest(
-      AnalysisUpdateContentParams({
-        testFile.path: AddContentOverlay(''),
-      }).toRequest('0'),
-    );
-
-    // Change the file to trigger the watcher. Since the request above should
-    // have cancelled (and removed) the timer, this should not do anything
-    // (specifically, it should not remove the new overlay).
-    modifyFile(testFilePath, 'error');
-
-    // The overlay should still be present, so we should be able to change it.
-    await handleSuccessfulRequest(
-      AnalysisUpdateContentParams({
-        testFile.path: ChangeContentOverlay(
-          [SourceEdit(0, 0, '//')],
-        ),
-      }).toRequest('0'),
-    );
-  }
-
   Future<void> test_setPriorityFiles() async {
     var a = getFile('$workspaceRootPath/foo/lib/a.dart');
     var b = getFile('$workspaceRootPath/foo/lib/b.dart');