Don't refresh dynamic files under a lock (#78775)

When we were getting the notification of a dynamic file being changed,
we were taking the project and workspace locks before fetching the
actual content. This is really expensive now that the fetching itself
might not be instant.

Fundamentally this change is just switching the work to acquire the
locks to get the setup info, then releasing the locks and calling the
provider to get the info. I'm putting this around a batching queue so we
can deduplicate lots of events at once, and also to ensure ordering so
if we were to try refreshing the document more than once we don't end up
in a case where we have two threads trying to put different versions of
the document into the workspace and getting the ordering wrong.

Fixes #78734

Author: jasonmalinowski

src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.BatchingDocumentCollection.cs

@@ -15,6 +15,7 @@
 using Microsoft.CodeAnalysis.PooledObjects;
 using Microsoft.CodeAnalysis.Shared.Extensions;
 using Microsoft.CodeAnalysis.Text;
+using Microsoft.CodeAnalysis.Threading;
 using Roslyn.Utilities;
 
 namespace Microsoft.CodeAnalysis.Workspaces.ProjectSystem;
@@ -69,6 +70,18 @@ private sealed class BatchingDocumentCollection
         private readonly Func<Solution, DocumentId, TextLoader, Solution> _documentTextLoaderChangedAction;
         private readonly WorkspaceChangeKind _documentChangedWorkspaceKind;
 
+        /// <summary>
+        /// An <see cref="AsyncBatchingWorkQueue"/> for processing updates to dynamic files. This is lazily created the first time we see
+        /// a change to process, since dynamic files are only used in certain Razor scenarios and most projects won't ever have one.
+        /// </summary>
+        /// <remarks>
+        /// This is used for two reasons: first, if we have a flurry of events we want to deduplicate them. But it also ensures ordering -- if we were to get a change to
+        /// a dynamic file while we're already processing another change, we want to ensure that the first change is processed before the second one. Otherwise we might
+        /// end up with the updates being applied out of order (since we're not always holding a lock while calling to the dynamic file info provider) and we might end up with
+        /// an old version stuck in the workspace.
+        /// </remarks>
+        private AsyncBatchingWorkQueue<(string projectSystemFilePath, string workspaceFilePath)>? _dynamicFilesToRefresh;
+
         public BatchingDocumentCollection(ProjectSystemProject project,
             Func<Solution, DocumentId, bool> documentAlreadyInWorkspace,
             Action<Workspace, DocumentInfo> documentAddAction,
@@ -213,7 +226,7 @@ public void AddDynamicFile_NoLock(IDynamicFileInfoProvider fileInfoProvider, Dyn
 
             _documentIdToDynamicFileInfoProvider.Add(documentId, fileInfoProvider);
 
-            if (_project._eventSubscriptionTracker.Add(fileInfoProvider))
+            if (_project._dynamicFileInfoProvidersSubscribedTo.Add(fileInfoProvider))
             {
                 // subscribe to the event when we use this provider the first time
                 fileInfoProvider.Updated += _project.OnDynamicFileInfoUpdated;
@@ -439,55 +452,66 @@ await _project._projectSystemProjectFactory.ApplyBatchChangeToWorkspaceAsync((so
         /// <summary>
         /// Process file content changes
         /// </summary>
-        /// <param name="projectSystemFilePath">filepath given from project system</param>
-        /// <param name="workspaceFilePath">filepath used in workspace. it might be different than projectSystemFilePath</param>
+        /// <param name="projectSystemFilePath">File path given from project system for the .cshtml file</param>
+        /// <param name="workspaceFilePath">File path for the equivalent .cs document used in workspace. it might be different than projectSystemFilePath.</param>
         public void ProcessDynamicFileChange(string projectSystemFilePath, string workspaceFilePath)
         {
-            using (_project._gate.DisposableWait())
+            InterlockedOperations.Initialize(ref _dynamicFilesToRefresh, () =>
             {
-                // If our project has already been removed, this is a stale notification, and we can disregard.
-                if (_project.HasBeenRemoved)
-                {
-                    return;
-                }
+                return new AsyncBatchingWorkQueue<(string, string)>(
+                    TimeSpan.FromMilliseconds(200), // 200 chosen with absolutely no evidence whatsoever
+                    ProcessDynamicFileChangesAsync,
+                    EqualityComparer<(string, string)>.Default, // uses ordinal string comparison which is what we want
+                    _project._projectSystemProjectFactory.WorkspaceListener,
+                    _project._asynchronousFileChangeProcessingCancellationTokenSource.Token);
+            });
+
+            _dynamicFilesToRefresh.AddWork((projectSystemFilePath, workspaceFilePath));
+        }
+
+        private async ValueTask ProcessDynamicFileChangesAsync(ImmutableSegmentedList<(string projectSystemFilePath, string workspaceFilePath)> batch, CancellationToken cancellationToken)
+        {
+            foreach (var (projectSystemPath, workspaceFilePath) in batch)
+            {
+                DocumentId? documentId;
+                IDynamicFileInfoProvider? fileInfoProvider;
 
-                if (_documentPathsToDocumentIds.TryGetValue(workspaceFilePath, out var documentId))
+                using (await _project._gate.DisposableWaitAsync(cancellationToken).ConfigureAwait(false))
                 {
-                    // We create file watching prior to pushing the file to the workspace in batching, so it's
-                    // possible we might see a file change notification early. In this case, toss it out. Since
-                    // all adds/removals of documents for this project happen under our lock, it's safe to do this
-                    // check without taking the main workspace lock. We don't have to check for documents removed in
-                    // the batch, since those have already been removed out of _documentPathsToDocumentIds.
-                    if (_documentsAddedInBatch.Any(d => d.Id == documentId))
-                    {
+                    // If our project has already been removed, this is a stale notification, and we can disregard.
+                    if (_project.HasBeenRemoved)
                         return;
-                    }
 
-                    Contract.ThrowIfFalse(_documentIdToDynamicFileInfoProvider.TryGetValue(documentId, out var fileInfoProvider));
+                    if (!_documentPathsToDocumentIds.TryGetValue(workspaceFilePath, out documentId))
+                        return;
 
-                    _project._projectSystemProjectFactory.ApplyChangeToWorkspace(w =>
+                    if (!_documentIdToDynamicFileInfoProvider.TryGetValue(documentId, out fileInfoProvider))
+                        return;
+                }
+
+                // Now that we've got all our basic data, let's fetch the new document outside the lock, since this could be expensive.
+                var fileInfo = await fileInfoProvider.GetDynamicFileInfoAsync(
+                    _project.Id, _project._filePath, projectSystemPath, CancellationToken.None).ConfigureAwait(false);
+                Contract.ThrowIfNull(fileInfo, "We previously received a dynamic file for this path, and we're responding to a change, so we expect to get a new one.");
+
+                await _project._projectSystemProjectFactory.ApplyChangeToWorkspaceAsync(w =>
                     {
                         if (w.IsDocumentOpen(documentId))
                         {
                             return;
                         }
 
-                        // we do not expect JTF to be used around this code path. and contract of fileInfoProvider is it being real free-threaded
-                        // meaning it can't use JTF to go back to UI thread.
-                        // so, it is okay for us to call regular ".Result" on a task here.
-                        var fileInfo = fileInfoProvider.GetDynamicFileInfoAsync(
-                            _project.Id, _project._filePath, projectSystemFilePath, CancellationToken.None).WaitAndGetResult_CanCallOnBackground(CancellationToken.None);
-
-                        Contract.ThrowIfNull(fileInfo, "We previously received a dynamic file for this path, and we're responding to a change, so we expect to get a new one.");
+                        // Right now we're only supporting dynamic files as actual source files, so it's OK to call GetDocument here.
+                        // If the document is longer present, that could mean we unloaded the project, or the dynamic file was removed while we had released the lock.
+                        var documentToReload = w.CurrentSolution.GetDocument(documentId);
 
-                        // Right now we're only supporting dynamic files as actual source files, so it's OK to call GetDocument here
-                        var attributes = w.CurrentSolution.GetRequiredDocument(documentId).State.Attributes;
+                        if (documentToReload is null)
+                            return;
 
-                        var documentInfo = new DocumentInfo(attributes, fileInfo.TextLoader, fileInfo.DocumentServiceProvider);
+                        var documentInfo = new DocumentInfo(documentToReload.State.Attributes, fileInfo.TextLoader, fileInfo.DocumentServiceProvider);
 
                         w.OnDocumentReloaded(documentInfo);
-                    });
-                }
+                    }, cancellationToken).ConfigureAwait(false);
             }
         }
 

src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs

@@ -124,9 +124,9 @@ internal sealed partial class ProjectSystemProject
     private readonly IFileChangeContext _documentFileChangeContext;
 
     /// <summary>
-    /// track whether we have been subscribed to <see cref="IDynamicFileInfoProvider.Updated"/> event
+    /// The set of dynamic file info providers we have already subscribed to.
     /// </summary>
-    private readonly HashSet<IDynamicFileInfoProvider> _eventSubscriptionTracker = [];
+    private readonly HashSet<IDynamicFileInfoProvider> _dynamicFileInfoProvidersSubscribedTo = [];
 
     /// <summary>
     /// Map of the original dynamic file path to the <see cref="DynamicFileInfo.FilePath"/> that was associated with it.
@@ -1417,12 +1417,12 @@ public void RemoveFromWorkspace()
             _asynchronousFileChangeProcessingCancellationTokenSource.Cancel();
 
             // clear tracking to external components
-            foreach (var provider in _eventSubscriptionTracker)
+            foreach (var provider in _dynamicFileInfoProvidersSubscribedTo)
             {
                 provider.Updated -= OnDynamicFileInfoUpdated;
             }
 
-            _eventSubscriptionTracker.Clear();
+            _dynamicFileInfoProvidersSubscribedTo.Clear();
         }
 
         _documentFileChangeContext.Dispose();