From 29561bfbe37e205db65643c2b86bf3a64a9ff854 Mon Sep 17 00:00:00 2001 From: Dustin Campbell Date: Wed, 24 Apr 2024 10:52:44 -0700 Subject: [PATCH] Don't use batching work queue in ProjectConfigurationStateSynchronizer Fixes #10306 The recent change to use an `AsyncBatchingWorkQueue` in `ProjectConfigurationStateSynchronizer` has resulted in a signficant regression. The syncrhonizer has somewhat subtle semantics that doesn't mesh well with `AsyncBatchingWorkQueue`. As part of rolling this change back, I've commented the code a bit more thoroughly to call out how it works and where the semantics are a bit flaky. Ultimately, I expect we'll be able to get rid of this once we stop writing `project.razor.vs.bin` files to disk. --- ...ConfigurationStateSynchronizer.Comparer.cs | 58 ---- ...igurationStateSynchronizer.TestAccessor.cs | 37 ++- .../ProjectConfigurationStateSynchronizer.cs | 256 ++++++++++++------ ...ojectConfigurationStateSynchronizerTest.cs | 26 +- 4 files changed, 220 insertions(+), 157 deletions(-) delete mode 100644 src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/ProjectConfigurationStateSynchronizer.Comparer.cs diff --git a/src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/ProjectConfigurationStateSynchronizer.Comparer.cs b/src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/ProjectConfigurationStateSynchronizer.Comparer.cs deleted file mode 100644 index fe7834af468..00000000000 --- a/src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/ProjectConfigurationStateSynchronizer.Comparer.cs +++ /dev/null @@ -1,58 +0,0 @@ -// Copyright (c) .NET Foundation. All rights reserved. -// Licensed under the MIT license. See License.txt in the project root for license information. - -using System.Collections.Generic; -using Microsoft.CodeAnalysis.Razor; - -namespace Microsoft.AspNetCore.Razor.LanguageServer; - -internal partial class ProjectConfigurationStateSynchronizer -{ - private sealed class Comparer : IEqualityComparer - { - public static readonly Comparer Instance = new(); - - private Comparer() - { - } - - public bool Equals(Work? x, Work? y) - { - if (x is null) - { - return y is null; - } - else if (y is null) - { - return x is null; - } - - // For purposes of removing duplicates from batches, two Work instances - // are equal only if their identifying properties are equal. So, only - // configuration file paths and project keys. - - if (!FilePathComparer.Instance.Equals(x.ConfigurationFilePath, y.ConfigurationFilePath)) - { - return false; - } - - return (x, y) switch - { - (AddProject, AddProject) => true, - - (ResetProject { ProjectKey: var keyX }, - ResetProject { ProjectKey: var keyY }) - => keyX == keyY, - - (UpdateProject { ProjectKey: var keyX }, - UpdateProject { ProjectKey: var keyY }) - => keyX == keyY, - - _ => false, - }; - } - - public int GetHashCode(Work obj) - => obj.GetHashCode(); - } -} diff --git a/src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/ProjectConfigurationStateSynchronizer.TestAccessor.cs b/src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/ProjectConfigurationStateSynchronizer.TestAccessor.cs index 5be787a4d8b..4f77a54b363 100644 --- a/src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/ProjectConfigurationStateSynchronizer.TestAccessor.cs +++ b/src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/ProjectConfigurationStateSynchronizer.TestAccessor.cs @@ -1,7 +1,10 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the MIT license. See License.txt in the project root for license information. +using System.Collections.Generic; +using System.Threading; using System.Threading.Tasks; +using Microsoft.VisualStudio.Threading; namespace Microsoft.AspNetCore.Razor.LanguageServer; @@ -11,7 +14,37 @@ internal partial class ProjectConfigurationStateSynchronizer internal sealed class TestAccessor(ProjectConfigurationStateSynchronizer instance) { - public Task WaitUntilCurrentBatchCompletesAsync() - => instance._workQueue.WaitUntilCurrentBatchCompletesAsync(); + public Task WaitForProjectUpdatesToDrainAsync(CancellationToken cancellationToken) + { + return Task.Run(async () => + { + while (UpdatesRemaining() > 0) + { + await Task.Yield(); + } + + int UpdatesRemaining() + { + lock (instance._projectUpdates) + { + var count = instance._projectUpdates.Count; + + if (count > 0) + { + foreach (var (_, updateItem) in instance._projectUpdates) + { + if (updateItem.Task is null || updateItem.Task.IsCompleted) + { + count--; + } + } + } + + return count; + } + } + }, + cancellationToken); + } } } diff --git a/src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/ProjectConfigurationStateSynchronizer.cs b/src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/ProjectConfigurationStateSynchronizer.cs index 6b585cf7d0c..66ace53d001 100644 --- a/src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/ProjectConfigurationStateSynchronizer.cs +++ b/src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/ProjectConfigurationStateSynchronizer.cs @@ -21,23 +21,54 @@ namespace Microsoft.AspNetCore.Razor.LanguageServer; +/// +/// +/// This service has complex and potentially flaky(!) semantics. When notified of project configuration file +/// adds, removes, and changes, it works to synchronize those changes with the . +/// This is handled like so: +/// +/// +/// +/// +/// When a configuration file is added, an attempt is made to add the project to the +/// . This might ultimately be a no-op if the project already exists, +/// but it'll return the in either case. This project key is added to +/// and used to enqueue an update to the project with the +/// that was deserialized from the configuration file. +/// +/// +/// +/// When a configuration file is changed, is used to +/// retrieve the . This key is then used to enqueue an update to the project +/// the deserialized . +/// +/// +/// +/// When a configuration file is removed, it is removed from +/// and its (if we knew about it) will be used to enqueue an update to reset the project +/// to an empty, unconfigured state. +/// +/// +/// +/// +/// Project updates are processed after a delay. The machinery ensures, that if an update for a project is already +/// in flight when a new update is enqueued, the project info will be replaced with the latest. This debouncing +/// accounts for scenarios where the configuration file is updated in rapid succession. +/// +/// internal partial class ProjectConfigurationStateSynchronizer : IProjectConfigurationFileChangeListener, IDisposable { - private abstract record Work(string ConfigurationFilePath); - private sealed record AddProject(string ConfigurationFilePath, RazorProjectInfo ProjectInfo) : Work(ConfigurationFilePath); - private sealed record ResetProject(string ConfigurationFilePath, ProjectKey ProjectKey) : Work(ConfigurationFilePath); - private sealed record UpdateProject(string ConfigurationFilePath, ProjectKey ProjectKey, RazorProjectInfo ProjectInfo) : Work(ConfigurationFilePath); - private static readonly TimeSpan s_delay = TimeSpan.FromMilliseconds(250); private readonly IRazorProjectService _projectService; private readonly LanguageServerFeatureOptions _options; private readonly ILogger _logger; + private readonly TimeSpan _delay; private readonly CancellationTokenSource _disposeTokenSource; - private readonly AsyncBatchingWorkQueue _workQueue; + private readonly Dictionary _projectUpdates = []; - private ImmutableDictionary _filePathToProjectKeyMap = + private ImmutableDictionary _configurationFileToProjectKeyMap = ImmutableDictionary.Empty.WithComparers(keyComparer: FilePathComparer.Instance); public ProjectConfigurationStateSynchronizer( @@ -57,9 +88,9 @@ protected ProjectConfigurationStateSynchronizer( _projectService = projectService; _options = options; _logger = loggerFactory.GetOrCreateLogger(); + _delay = delay; _disposeTokenSource = new(); - _workQueue = new(delay, ProcessBatchAsync, _disposeTokenSource.Token); } public void Dispose() @@ -67,74 +98,6 @@ public void Dispose() _disposeTokenSource.Cancel(); _disposeTokenSource.Dispose(); } - private async ValueTask ProcessBatchAsync(ImmutableArray items, CancellationToken token) - { - foreach (var item in items.GetMostRecentUniqueItems(Comparer.Instance)) - { - var itemTask = item switch - { - AddProject(var configurationFilePath, var projectInfo) => AddProjectAsync(configurationFilePath, projectInfo, token), - ResetProject(_, var projectKey) => ResetProjectAsync(projectKey, token), - UpdateProject(_, var projectKey, var projectInfo) => UpdateProjectAsync(projectKey, projectInfo, token), - _ => Assumed.Unreachable() - }; - - await itemTask.ConfigureAwait(false); - } - - async Task AddProjectAsync(string configurationFilePath, RazorProjectInfo projectInfo, CancellationToken token) - { - var projectFilePath = FilePathNormalizer.Normalize(projectInfo.FilePath); - var intermediateOutputPath = FilePathNormalizer.GetNormalizedDirectoryName(configurationFilePath); - - var projectKey = await _projectService - .AddProjectAsync( - projectFilePath, - intermediateOutputPath, - projectInfo.Configuration, - projectInfo.RootNamespace, - projectInfo.DisplayName, - token) - .ConfigureAwait(false); - - _logger.LogInformation($"Added {projectKey.Id}."); - - ImmutableInterlocked.AddOrUpdate(ref _filePathToProjectKeyMap, configurationFilePath, projectKey, static (k, v) => v); - _logger.LogInformation($"Project configuration file added for project '{projectFilePath}': '{configurationFilePath}'"); - - await UpdateProjectAsync(projectKey, projectInfo, token).ConfigureAwait(false); - } - - Task ResetProjectAsync(ProjectKey projectKey, CancellationToken token) - { - _logger.LogInformation($"Resetting {projectKey.Id}."); - - return _projectService - .UpdateProjectAsync( - projectKey, - configuration: null, - rootNamespace: null, - displayName: "", - ProjectWorkspaceState.Default, - documents: [], - token); - } - - Task UpdateProjectAsync(ProjectKey projectKey, RazorProjectInfo projectInfo, CancellationToken token) - { - _logger.LogInformation($"Updating {projectKey.Id}."); - - return _projectService - .UpdateProjectAsync( - projectKey, - projectInfo.Configuration, - projectInfo.RootNamespace, - projectInfo.DisplayName, - projectInfo.ProjectWorkspaceState, - projectInfo.Documents, - token); - } - } public void ProjectConfigurationFileChanged(ProjectConfigurationFileChangeEventArgs args) { @@ -146,14 +109,14 @@ public void ProjectConfigurationFileChanged(ProjectConfigurationFileChangeEventA { if (args.TryDeserialize(_options, out var projectInfo)) { - if (_filePathToProjectKeyMap.TryGetValue(configurationFilePath, out var projectKey)) + if (_configurationFileToProjectKeyMap.TryGetValue(configurationFilePath, out var projectKey)) { _logger.LogInformation($""" Configuration file changed for project '{projectKey.Id}'. Configuration file path: '{configurationFilePath}' """); - _workQueue.AddWork(new UpdateProject(configurationFilePath, projectKey, projectInfo)); + EnqueueProjectUpdate(projectKey, projectInfo, _disposeTokenSource.Token); } else { @@ -162,12 +125,12 @@ public void ProjectConfigurationFileChanged(ProjectConfigurationFileChangeEventA Configuration file path: '{configurationFilePath}' """); - _workQueue.AddWork(new AddProject(configurationFilePath, projectInfo)); + AddProject(configurationFilePath, projectInfo, _disposeTokenSource.Token); } } else { - if (_filePathToProjectKeyMap.TryGetValue(configurationFilePath, out var projectKey)) + if (_configurationFileToProjectKeyMap.TryGetValue(configurationFilePath, out var projectKey)) { _logger.LogWarning($""" Failed to deserialize after change to configuration file for project '{projectKey.Id}'. @@ -177,7 +140,7 @@ public void ProjectConfigurationFileChanged(ProjectConfigurationFileChangeEventA // We found the last associated project file for the configuration file. Reset the project since we can't // accurately determine its configurations. - _workQueue.AddWork(new ResetProject(configurationFilePath, projectKey)); + EnqueueProjectReset(projectKey, _disposeTokenSource.Token); } else { @@ -196,7 +159,7 @@ Failed to deserialize after change to previously unseen configuration file. { if (args.TryDeserialize(_options, out var projectInfo)) { - _workQueue.AddWork(new AddProject(configurationFilePath, projectInfo)); + AddProject(configurationFilePath, projectInfo, _disposeTokenSource.Token); } else { @@ -213,14 +176,14 @@ Failed to deserialize previously unseen configuration file. case RazorFileChangeKind.Removed: { - if (ImmutableInterlocked.TryRemove(ref _filePathToProjectKeyMap, configurationFilePath, out var projectKey)) + if (ImmutableInterlocked.TryRemove(ref _configurationFileToProjectKeyMap, configurationFilePath, out var projectKey)) { _logger.LogInformation($""" Configuration file removed for project '{projectKey}'. Configuration file path: '{configurationFilePath}' """); - _workQueue.AddWork(new ResetProject(configurationFilePath, projectKey)); + EnqueueProjectReset(projectKey, _disposeTokenSource.Token); } else { @@ -234,4 +197,129 @@ Failed to resolve associated project on configuration removed event. break; } } + + private void AddProject(string configurationFilePath, RazorProjectInfo projectInfo, CancellationToken cancellationToken) + { + // Note that we fire-and-forget adding the project here because it's an asynchronous operation. + // This means that the add will not happen immediately. However, once it does, we'll enqueue a project update. + // This *could* represent a race condition if another *newer* project update is enqueued after the project is + // added but before the project update for the add is enqueued. + AddProjectAsync(configurationFilePath, projectInfo, cancellationToken).Forget(); + + async Task AddProjectAsync(string configurationFilePath, RazorProjectInfo projectInfo, CancellationToken cancellationToken) + { + var projectFilePath = FilePathNormalizer.Normalize(projectInfo.FilePath); + var intermediateOutputPath = FilePathNormalizer.GetNormalizedDirectoryName(configurationFilePath); + + var projectKey = await _projectService + .AddProjectAsync( + projectFilePath, + intermediateOutputPath, + projectInfo.Configuration, + projectInfo.RootNamespace, + projectInfo.DisplayName, + cancellationToken) + .ConfigureAwait(false); + + ImmutableInterlocked.AddOrUpdate(ref _configurationFileToProjectKeyMap, configurationFilePath, projectKey, static (k, v) => v); + _logger.LogInformation($"Project configuration file added for project '{projectFilePath}': '{configurationFilePath}'"); + + EnqueueProjectUpdate(projectKey, projectInfo, cancellationToken); + } + } + + private void EnqueueProjectReset(ProjectKey projectKey, CancellationToken cancellationToken) + => EnqueueProjectUpdate(projectKey, projectInfo: null, cancellationToken); + + private void EnqueueProjectUpdate(ProjectKey projectKey, RazorProjectInfo? projectInfo, CancellationToken cancellationToken) + { + // Note: We lock on _projectUpdates to protect access to both _projectUpdates and UpdateItem. + lock (_projectUpdates) + { + if (!_projectUpdates.TryGetValue(projectKey, out var updateItem)) + { + updateItem = new(projectKey); + _projectUpdates.Add(projectKey, updateItem); + } + + // Ensure that we use the most recent project info, which could be null + // in the case of a reset. + updateItem.ProjectInfo = projectInfo; + + // If this is new or its task has completed, start a new task to update + // the project after a delay. Note that we don't provide the project info + // because it might be updated before the delay completes. + if (updateItem.Task is null || updateItem.Task.IsCompleted) + { + updateItem.Task = UpdateAfterDelayAsync(projectKey, cancellationToken); + } + } + + async Task UpdateAfterDelayAsync(ProjectKey projectKey, CancellationToken cancellationToken) + { + await Task.Delay(_delay, cancellationToken).ConfigureAwait(false); + + // First, retrieve the project info (if any) from the update item. + RazorProjectInfo? projectInfo; + + lock (_projectUpdates) + { + if (!_projectUpdates.TryGetValue(projectKey, out var updateItem)) + { + // The update item should not be removed before we've handled it here. + Assumed.Unreachable(); + return; + } + + projectInfo = updateItem.ProjectInfo; + } + + await UpdateProjectAsync(projectKey, projectInfo, cancellationToken).ConfigureAwait(false); + + // Now that the project update is complete, we can remove the update item, but + // be sure to protect access. + lock (_projectUpdates) + { + _projectUpdates.Remove(projectKey); + } + + Task UpdateProjectAsync(ProjectKey projectKey, RazorProjectInfo? projectInfo, CancellationToken cancellationToken) + { + if (projectInfo is null) + { + // When we're passed a null RazorProjectInfo, we reset the project back to an empty, unconfigured state. + return _projectService.UpdateProjectAsync( + projectKey, + configuration: null, + rootNamespace: null, + displayName: "", + ProjectWorkspaceState.Default, + documents: [], + cancellationToken); + } + + _logger.LogInformation($"Updating {projectKey} with real project info."); + + return _projectService.UpdateProjectAsync( + projectKey, + projectInfo.Configuration, + projectInfo.RootNamespace, + projectInfo.DisplayName, + projectInfo.ProjectWorkspaceState, + projectInfo.Documents, + cancellationToken); + } + } + } + + /// + /// This is used to track the running that + /// ultimately updates the project and the latest that + /// the project will be updated with. + /// + private sealed record UpdateItem(ProjectKey Key) + { + public Task? Task { get; set; } + public RazorProjectInfo? ProjectInfo { get; set; } + } } diff --git a/src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/ProjectConfigurationStateSynchronizerTest.cs b/src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/ProjectConfigurationStateSynchronizerTest.cs index e56723bde84..0a67dfbd7c1 100644 --- a/src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/ProjectConfigurationStateSynchronizerTest.cs +++ b/src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/ProjectConfigurationStateSynchronizerTest.cs @@ -44,7 +44,7 @@ public async Task ProjectConfigurationFileChanged_Removed_UnknownDocumentNoops() // Act synchronizer.ProjectConfigurationFileChanged(args); - await synchronizerAccessor.WaitUntilCurrentBatchCompletesAsync(); + await synchronizerAccessor.WaitForProjectUpdatesToDrainAsync(DisposalToken); // Assert projectServiceMock.VerifyAll(); @@ -108,7 +108,7 @@ public async Task ProjectConfigurationFileChanged_Removed_NonNormalizedPaths() synchronizer.ProjectConfigurationFileChanged(addArgs); - await synchronizerAccessor.WaitUntilCurrentBatchCompletesAsync(); + await synchronizerAccessor.WaitForProjectUpdatesToDrainAsync(DisposalToken); var removeArgs = new ProjectConfigurationFileChangeEventArgs( configurationFilePath: "/path/to/obj/project.razor.bin", @@ -118,7 +118,7 @@ public async Task ProjectConfigurationFileChanged_Removed_NonNormalizedPaths() // Act synchronizer.ProjectConfigurationFileChanged(removeArgs); - await synchronizerAccessor.WaitUntilCurrentBatchCompletesAsync(); + await synchronizerAccessor.WaitForProjectUpdatesToDrainAsync(DisposalToken); // Assert projectServiceMock.VerifyAll(); @@ -141,7 +141,7 @@ public async Task ProjectConfigurationFileChanged_Added_CantDeserialize_Noops() // Act synchronizer.ProjectConfigurationFileChanged(args); - await synchronizerAccessor.WaitUntilCurrentBatchCompletesAsync(); + await synchronizerAccessor.WaitForProjectUpdatesToDrainAsync(DisposalToken); // Assert projectServiceMock.VerifyAll(); @@ -195,7 +195,7 @@ public async Task ProjectConfigurationFileChanged_Added_AddAndUpdatesProject() // Act synchronizer.ProjectConfigurationFileChanged(args); - await synchronizerAccessor.WaitUntilCurrentBatchCompletesAsync(); + await synchronizerAccessor.WaitForProjectUpdatesToDrainAsync(DisposalToken); // Assert projectServiceMock.VerifyAll(); @@ -259,7 +259,7 @@ public async Task ProjectConfigurationFileChanged_Removed_ResetsProject() synchronizer.ProjectConfigurationFileChanged(addArgs); - await synchronizerAccessor.WaitUntilCurrentBatchCompletesAsync(); + await synchronizerAccessor.WaitForProjectUpdatesToDrainAsync(DisposalToken); var removeArgs = new ProjectConfigurationFileChangeEventArgs( configurationFilePath: "/path/to/obj/project.razor.bin", @@ -269,7 +269,7 @@ public async Task ProjectConfigurationFileChanged_Removed_ResetsProject() // Act synchronizer.ProjectConfigurationFileChanged(removeArgs); - await synchronizerAccessor.WaitUntilCurrentBatchCompletesAsync(); + await synchronizerAccessor.WaitForProjectUpdatesToDrainAsync(DisposalToken); // Assert projectServiceMock.VerifyAll(); @@ -343,7 +343,7 @@ public async Task ProjectConfigurationFileChanged_Changed_UpdatesProject() synchronizer.ProjectConfigurationFileChanged(addArgs); - await synchronizerAccessor.WaitUntilCurrentBatchCompletesAsync(); + await synchronizerAccessor.WaitForProjectUpdatesToDrainAsync(DisposalToken); var changedArgs = new ProjectConfigurationFileChangeEventArgs( configurationFilePath: "/path/to/obj/project.razor.bin", @@ -353,7 +353,7 @@ public async Task ProjectConfigurationFileChanged_Changed_UpdatesProject() // Act synchronizer.ProjectConfigurationFileChanged(changedArgs); - await synchronizerAccessor.WaitUntilCurrentBatchCompletesAsync(); + await synchronizerAccessor.WaitForProjectUpdatesToDrainAsync(DisposalToken); // Assert projectServiceMock.VerifyAll(); @@ -429,7 +429,7 @@ public async Task ProjectConfigurationFileChanged_Changed_CantDeserialize_Resets synchronizer.ProjectConfigurationFileChanged(addArgs); - await synchronizerAccessor.WaitUntilCurrentBatchCompletesAsync(); + await synchronizerAccessor.WaitForProjectUpdatesToDrainAsync(DisposalToken); var changedArgs = new ProjectConfigurationFileChangeEventArgs( configurationFilePath: "/path/to/obj/project.razor.bin", @@ -439,7 +439,7 @@ public async Task ProjectConfigurationFileChanged_Changed_CantDeserialize_Resets // Act synchronizer.ProjectConfigurationFileChanged(changedArgs); - await synchronizerAccessor.WaitUntilCurrentBatchCompletesAsync(); + await synchronizerAccessor.WaitForProjectUpdatesToDrainAsync(DisposalToken); // Assert projectServiceMock.VerifyAll(); @@ -462,7 +462,7 @@ public async Task ProjectConfigurationFileChanged_Changed_UntrackedProject_Noops // Act synchronizer.ProjectConfigurationFileChanged(changedArgs); - await synchronizerAccessor.WaitUntilCurrentBatchCompletesAsync(); + await synchronizerAccessor.WaitForProjectUpdatesToDrainAsync(DisposalToken); // Assert projectService.VerifyAll(); @@ -515,7 +515,7 @@ public async Task ProjectConfigurationFileChanged_RemoveThenAdd_OnlyAdds() synchronizer.ProjectConfigurationFileChanged(addedArgs); synchronizer.ProjectConfigurationFileChanged(changedArgs); - await synchronizerAccessor.WaitUntilCurrentBatchCompletesAsync(); + await synchronizerAccessor.WaitForProjectUpdatesToDrainAsync(DisposalToken); // Assert projectServiceMock.Verify(p => p.UpdateProjectAsync(