Send watch events through the message scheduler

If we need to control when requests that modify the state of analysis
are performed, and we do, then that needs to include watch events from
a file watcher. This CL causes those events to be placed on the message
scheduler's queue so that we can control when we notify the analysis
driver of changes.

Change-Id: I1b099a35da7a4b9a6f4d86feb497d6822041155d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/421520
Reviewed-by: Phil Quitslund <[email protected]>
Commit-Queue: Brian Wilkerson <[email protected]>

Author: bwilkerson

pkg/analysis_server/lib/src/analysis_server.dart

@@ -1062,8 +1062,6 @@ abstract class CommonServerContextManagerCallbacks
 
   CommonServerContextManagerCallbacks(this.resourceProvider);
 
-  AnalysisServer get analysisServer;
-
   @override
   @mustCallSuper
   void afterContextsCreated() {

pkg/analysis_server/lib/src/context_manager.dart

@@ -5,7 +5,9 @@
 import 'dart:async';
 import 'dart:collection';
 
+import 'package:analysis_server/src/analysis_server.dart';
 import 'package:analysis_server/src/lsp/handlers/handlers.dart';
+import 'package:analysis_server/src/scheduler/scheduled_message.dart';
 import 'package:analysis_server/src/services/correction/fix/data_driven/transform_set_parser.dart';
 import 'package:analyzer/dart/analysis/analysis_context.dart';
 import 'package:analyzer/dart/analysis/features.dart';
@@ -91,6 +93,9 @@ abstract class ContextManager {
   /// If no driver contains the given path, `null` is returned.
   AnalysisDriver? getDriverFor(String path);
 
+  /// Handle an [event] from a file watcher.
+  void handleWatchEvent(WatchEvent event);
+
   /// Return `true` if the file or directory with the given [path] will be
   /// analyzed in one of the analysis contexts.
   bool isAnalyzed(String path);
@@ -123,6 +128,9 @@ abstract class ContextManager {
 // operations return data structures describing how context state should be
 // modified.
 abstract class ContextManagerCallbacks {
+  /// The analysis server that created this callback object.
+  AnalysisServer get analysisServer;
+
   /// Called after analysis contexts are created, usually when new analysis
   /// roots are set, or after detecting a change that required rebuilding
   /// the set of analysis contexts.
@@ -140,7 +148,7 @@ abstract class ContextManagerCallbacks {
   /// Sent the given watch [event] to any interested plugins.
   void broadcastWatchEvent(WatchEvent event);
 
-  /// Invoked on any [FileResult] in the analyzer events stream.
+  /// Invoked on every [FileResult] in the analyzer events stream.
   void handleFileResult(FileResult result);
 
   /// Add listeners to the [driver]. This must be the only listener.
@@ -309,6 +317,13 @@ class ContextManagerImpl implements ContextManager {
     return getContextFor(path)?.driver;
   }
 
+  @override
+  void handleWatchEvent(WatchEvent event) {
+    callbacks.broadcastWatchEvent(event);
+    _handleWatchEvent(event);
+    callbacks.afterWatchEvent(event);
+  }
+
   @override
   bool isAnalyzed(String path) {
     var collection = _collection;
@@ -610,7 +625,7 @@ class ContextManagerImpl implements ContextManager {
             watchers.add(watcher);
             watcherSubscriptions.add(
               watcher.changes.listen(
-                _handleWatchEvent,
+                _scheduleWatchEvent,
                 onError: _handleWatchInterruption,
               ),
             );
@@ -808,18 +823,14 @@ class ContextManagerImpl implements ContextManager {
     }
   }
 
+  /// Handle an [event] from a file watcher.
   void _handleWatchEvent(WatchEvent event) {
-    callbacks.broadcastWatchEvent(event);
-    _handleWatchEventImpl(event);
-    callbacks.afterWatchEvent(event);
-  }
-
-  void _handleWatchEventImpl(WatchEvent event) {
     // Figure out which context this event applies to.
-    // TODO(brianwilkerson): If a file is explicitly included in one context
-    // but implicitly referenced in another context, we will only send a
-    // changeSet to the context that explicitly includes the file (because
-    // that's the only context that's watching the file).
+    //
+    // If a file is explicitly included in one context but implicitly referenced
+    // in another context, we will only notify the context that explicitly
+    // includes the file (because that's the only context that's watching the
+    // file).
     var path = event.path;
     var type = event.type;
 
@@ -915,6 +926,15 @@ class ContextManagerImpl implements ContextManager {
         existingExcludedSet.containsAll(excludedPaths);
   }
 
+  /// Schedule the handling of a watch event.
+  ///
+  /// This places the watch event on the message scheduler's queue so that it
+  /// can be processed when we're not in the middle of handling some other
+  /// message.
+  void _scheduleWatchEvent(WatchEvent event) {
+    callbacks.analysisServer.messageScheduler.add(WatcherMessage(event));
+  }
+
   /// Listens to files generated by Blaze that were found or searched for.
   ///
   /// This is handled specially because the files are outside the package
@@ -941,6 +961,12 @@ class ContextManagerImpl implements ContextManager {
 }
 
 class NoopContextManagerCallbacks implements ContextManagerCallbacks {
+  @override
+  AnalysisServer get analysisServer =>
+      throw StateError(
+        'The callback object should have been set by the server.',
+      );
+
   @override
   void afterContextsCreated() {}
 

pkg/analysis_server/lib/src/scheduler/message_scheduler.dart

@@ -231,6 +231,11 @@ final class MessageScheduler {
               message.responseCompleter,
               completer,
             );
+          case WatcherMessage():
+            server.contextManager.handleWatchEvent(message.event);
+            // Handling a watch event is a synchronous process, so there's
+            // nothing to wait for.
+            completer.complete();
         }
 
         // Blocking here with an await on the future was intended to prevent

pkg/analysis_server/lib/src/scheduler/scheduled_message.dart

@@ -9,11 +9,18 @@ import 'package:analysis_server/protocol/protocol.dart' as legacy;
 import 'package:analyzer/src/util/performance/operation_performance.dart';
 import 'package:analyzer/src/utilities/cancellation.dart';
 import 'package:language_server_protocol/protocol_custom_generated.dart';
+import 'package:watcher/watcher.dart';
 
 /// Represents a message from DTD (Dart Tooling Daemon).
 final class DtdMessage extends ScheduledMessage {
+  /// The message that was received.
   final lsp.IncomingMessage message;
+
+  /// The completer to be signalled when a response to the message has been
+  /// computed.
   final Completer<Map<String, Object?>> responseCompleter;
+
+  /// The object used to gather performance data.
   final OperationPerformanceImpl performance;
 
   DtdMessage({
@@ -28,7 +35,11 @@ final class DtdMessage extends ScheduledMessage {
 
 /// Represents a message in the Legacy protocol format.
 final class LegacyMessage extends ScheduledMessage {
+  /// The Legacy message that was received.
   final legacy.Request request;
+
+  /// The token used to cancel the request, or `null` if the message is not a
+  /// request.
   CancelableToken? cancellationToken;
 
   LegacyMessage({required this.request, this.cancellationToken});
@@ -39,7 +50,11 @@ final class LegacyMessage extends ScheduledMessage {
 
 /// Represents a message in the LSP protocol format.
 final class LspMessage extends ScheduledMessage {
+  /// The LSP message that was received.
   final lsp.Message message;
+
+  /// The token used to cancel the request, or `null` if the message is not a
+  /// request.
   CancelableToken? cancellationToken;
 
   LspMessage({required this.message, this.cancellationToken});
@@ -64,3 +79,16 @@ final class LspMessage extends ScheduledMessage {
 ///
 /// The client can be an IDE, a command-line tool, or DTD.
 sealed class ScheduledMessage {}
+
+/// Represents a message from the file watcher.
+///
+/// This is always a notification.
+final class WatcherMessage extends ScheduledMessage {
+  /// The event that was received.
+  final WatchEvent event;
+
+  WatcherMessage(this.event);
+
+  @override
+  String toString() => '${event.type} ${event.path}';
+}