diff --git a/src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.BatchingDocumentCollection.cs b/src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.BatchingDocumentCollection.cs index 4bafa8a44f84b..c4771cc5212a1 100644 --- a/src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.BatchingDocumentCollection.cs +++ b/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 _documentTextLoaderChangedAction; private readonly WorkspaceChangeKind _documentChangedWorkspaceKind; + /// + /// An 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. + /// + /// + /// 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. + /// + private AsyncBatchingWorkQueue<(string projectSystemFilePath, string workspaceFilePath)>? _dynamicFilesToRefresh; + public BatchingDocumentCollection(ProjectSystemProject project, Func documentAlreadyInWorkspace, Action 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 /// /// Process file content changes /// - /// filepath given from project system - /// filepath used in workspace. it might be different than projectSystemFilePath + /// File path given from project system for the .cshtml file + /// File path for the equivalent .cs document used in workspace. it might be different than projectSystemFilePath. 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); } } diff --git a/src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs b/src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs index 4ac028169b435..a08fc177c1c92 100644 --- a/src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs +++ b/src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs @@ -124,9 +124,9 @@ internal sealed partial class ProjectSystemProject private readonly IFileChangeContext _documentFileChangeContext; /// - /// track whether we have been subscribed to event + /// The set of dynamic file info providers we have already subscribed to. /// - private readonly HashSet _eventSubscriptionTracker = []; + private readonly HashSet _dynamicFileInfoProvidersSubscribedTo = []; /// /// Map of the original dynamic file path to the 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();