Skip to content

Collapse Reset then Add to Update in project state synchronizer #10330

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
davidwengier wants to merge 3 commits into from
Closed

Collapse Reset then Add to Update in project state synchronizer #10330

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
130eeec
Collapse Reset then Add to Update in project state synchronizer
davidwengier Apr 24, 2024
90b56bd
Add more tests, and fix a bug found by the tests that were added
davidwengier Apr 30, 2024
0d46788
PR Feedback
davidwengier May 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...crosoft.AspNetCore.Razor.LanguageServer/ProjectConfigurationStateSynchronizer.Comparer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ public bool Equals(Work? x, Work? y)
// 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))
if (x.ProjectKey != y.ProjectKey)
{
return false;
}
Expand Down
160 changes: 87 additions & 73 deletions ...or/src/Microsoft.AspNetCore.Razor.LanguageServer/ProjectConfigurationStateSynchronizer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,13 @@
using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Diagnostics;
using System.IO;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Razor.LanguageServer.Common;
using Microsoft.AspNetCore.Razor.LanguageServer.ProjectSystem;
using Microsoft.AspNetCore.Razor.PooledObjects;
using Microsoft.AspNetCore.Razor.ProjectSystem;
using Microsoft.AspNetCore.Razor.Utilities;
using Microsoft.CodeAnalysis;
Expand All @@ -23,10 +25,10 @@ namespace Microsoft.AspNetCore.Razor.LanguageServer;

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 abstract record Work(ProjectKey ProjectKey);
private sealed record AddProject(RazorProjectInfo ProjectInfo) : Work(ProjectKey.From(ProjectInfo));
private sealed record ResetProject(ProjectKey ProjectKey) : Work(ProjectKey);
private sealed record UpdateProject(ProjectKey ProjectKey, RazorProjectInfo ProjectInfo) : Work(ProjectKey);

private static readonly TimeSpan s_delay = TimeSpan.FromMilliseconds(250);

Expand All @@ -37,8 +39,8 @@ private sealed record UpdateProject(string ConfigurationFilePath, ProjectKey Pro
private readonly CancellationTokenSource _disposeTokenSource;
private readonly AsyncBatchingWorkQueue<Work> _workQueue;

private ImmutableDictionary<string, ProjectKey> _filePathToProjectKeyMap =
ImmutableDictionary<string, ProjectKey>.Empty.WithComparers(keyComparer: FilePathComparer.Instance);
private readonly Dictionary<ProjectKey, (ResetProject work, int index)> _resetProjectMap = new();
private readonly HashSet<int> _indicesToSkip = new();

public ProjectConfigurationStateSynchronizer(
IRazorProjectService projectService,
Expand Down Expand Up @@ -69,24 +71,81 @@ public void Dispose()
}
private async ValueTask ProcessBatchAsync(ImmutableArray<Work> items, CancellationToken token)
{
// Clear out our helper collections.
_resetProjectMap.Clear();
_indicesToSkip.Clear();

using var itemsToProcess = new PooledArrayBuilder<Work>(items.Length);
var index = 0;

foreach (var item in items.GetMostRecentUniqueItems(Comparer.Instance))
{
if (token.IsCancellationRequested)
{
return;
}

if (item is ResetProject reset)
Comment thread
davidwengier marked this conversation as resolved.
Outdated
{
// We shouldn't ever get two resets for the same project, due to GetMostRecentUniqueItems, so the dictionary Add
// should always be safe
Debug.Assert(!_resetProjectMap.ContainsKey(reset.ProjectKey));

_resetProjectMap.Add(reset.ProjectKey, (reset, index));
itemsToProcess.Add(reset);
}
else if (item is AddProject add &&
_resetProjectMap.TryGetValue(add.ProjectKey, out var previousReset))
{
// We've already seen a Reset for this file path and now we have an Add, so let's convert to an Update
// and skip the Reset.
_indicesToSkip.Add(previousReset.index);

var update = new UpdateProject(previousReset.work.ProjectKey, add.ProjectInfo);
itemsToProcess.Add(update);
_resetProjectMap.Remove(add.ProjectKey);
}
else
{
itemsToProcess.Add(item);
}

index++;
}

// Now, loop through all of the file paths we collected and notify listeners of changes,
// taking care of to skip any indices that we noted earlier.
for (var i = 0; i < itemsToProcess.Count; i++)
{
if (token.IsCancellationRequested)
{
return;
}

if (_indicesToSkip.Contains(i))
{
continue;
}

var item = itemsToProcess[i];

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),
AddProject(var projectInfo) => AddProjectAsync(projectInfo, token),
ResetProject(var projectKey) => ResetProjectAsync(projectKey, token),
UpdateProject(var projectKey, var projectInfo) => UpdateProjectAsync(projectKey, projectInfo, token),
_ => Assumed.Unreachable<Task>()
};

await itemTask.ConfigureAwait(false);
}

async Task AddProjectAsync(string configurationFilePath, RazorProjectInfo projectInfo, CancellationToken token)
async Task AddProjectAsync(RazorProjectInfo projectInfo, CancellationToken token)
{
var projectFilePath = FilePathNormalizer.Normalize(projectInfo.FilePath);
var intermediateOutputPath = FilePathNormalizer.GetNormalizedDirectoryName(configurationFilePath);
var intermediateOutputPath = FilePathNormalizer.GetNormalizedDirectoryName(projectInfo.SerializedFilePath);

// If the project already exists, this will be a no-op
var projectKey = await _projectService
.AddProjectAsync(
projectFilePath,
Expand All @@ -99,8 +158,7 @@ async Task AddProjectAsync(string configurationFilePath, RazorProjectInfo projec

_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}'");
_logger.LogInformation($"Project configuration file added for project '{projectFilePath}': '{projectInfo.SerializedFilePath}'");

await UpdateProjectAsync(projectKey, projectInfo, token).ConfigureAwait(false);
}
Expand Down Expand Up @@ -138,55 +196,26 @@ Task UpdateProjectAsync(ProjectKey projectKey, RazorProjectInfo projectInfo, Can

public void ProjectConfigurationFileChanged(ProjectConfigurationFileChangeEventArgs args)
{
var configurationFilePath = FilePathNormalizer.Normalize(args.ConfigurationFilePath);

switch (args.Kind)
{
case RazorFileChangeKind.Changed:
{
if (args.TryDeserialize(_options, out var projectInfo))
{
if (_filePathToProjectKeyMap.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));
}
else
{
_logger.LogWarning($"""
Adding project for previously unseen configuration file.
Configuration file path: '{configurationFilePath}'
""");

_workQueue.AddWork(new AddProject(configurationFilePath, projectInfo));
}
var projectKey = ProjectKey.From(projectInfo);
_logger.LogInformation($"Configuration file changed for project '{projectKey.Id}'.");

// UpdateProject will no-op if the project isn't known
_workQueue.AddWork(new UpdateProject(projectKey, projectInfo));
}
else
{
if (_filePathToProjectKeyMap.TryGetValue(configurationFilePath, out var projectKey))
{
_logger.LogWarning($"""
Failed to deserialize after change to configuration file for project '{projectKey.Id}'.
Configuration file path: '{configurationFilePath}'
""");

// 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));
}
else
{
// Could not resolve an associated project file.
_logger.LogWarning($"""
Failed to deserialize after change to previously unseen configuration file.
Configuration file path: '{configurationFilePath}'
""");
}
var projectKey = ProjectKey.FromString(FilePathNormalizer.GetNormalizedDirectoryName(args.ConfigurationFilePath));

@davidwengier davidwengier Apr 30, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could do a ProjectKey.From(ProjectConfigurationFileChangeEventArgs) overload?

@DustinCampbell DustinCampbell Apr 30, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems a bit cumbersome to make the ProjectKey API dependent on ProjectConfigurationFileChangeEventArgs. I'd rather see a ProjectConfigurationFileChangeEventArgs.ToProjectKey() method.

@davidwengier davidwengier Apr 30, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, I should have thought of that!

_logger.LogWarning($"Failed to deserialize after change to configuration file for project '{projectKey.Id}'.");

// 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(projectKey));
}
}

Expand All @@ -196,39 +225,24 @@ Failed to deserialize after change to previously unseen configuration file.
{
if (args.TryDeserialize(_options, out var projectInfo))
{
_workQueue.AddWork(new AddProject(configurationFilePath, projectInfo));
_workQueue.AddWork(new AddProject(projectInfo));
}
else
{
// This is the first time we've seen this configuration file, but we can't deserialize it.
// The only thing we can really do is issue a warning.
_logger.LogWarning($"""
Failed to deserialize previously unseen configuration file.
Configuration file path: '{configurationFilePath}'
""");
_logger.LogWarning($"Failed to deserialize previously unseen configuration file '{args.ConfigurationFilePath}'");
}
}

break;

case RazorFileChangeKind.Removed:
{
if (ImmutableInterlocked.TryRemove(ref _filePathToProjectKeyMap, configurationFilePath, out var projectKey))
{
_logger.LogInformation($"""
Configuration file removed for project '{projectKey}'.
Configuration file path: '{configurationFilePath}'
""");
var projectKey = ProjectKey.FromString(FilePathNormalizer.GetNormalizedDirectoryName(args.ConfigurationFilePath));
_logger.LogInformation($"Configuration file removed for project '{projectKey}'.");

_workQueue.AddWork(new ResetProject(configurationFilePath, projectKey));
}
else
{
_logger.LogWarning($"""
Failed to resolve associated project on configuration removed event.
Configuration file path: '{configurationFilePath}'
""");
}
_workQueue.AddWork(new ResetProject(projectKey));
}

break;
Expand Down
2 changes: 2 additions & 0 deletions src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/ProjectKey.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using System.Diagnostics;
using Microsoft.AspNetCore.Razor.ProjectSystem;
using Microsoft.AspNetCore.Razor.Utilities;

namespace Microsoft.CodeAnalysis.Razor.ProjectSystem;
Expand All @@ -18,6 +19,7 @@ internal readonly record struct ProjectKey
// end up. All creation logic is here in one place to ensure this is consistent.
public static ProjectKey From(HostProject hostProject) => new(hostProject.IntermediateOutputPath);
public static ProjectKey From(IProjectSnapshot project) => new(project.IntermediateOutputPath);
public static ProjectKey From(RazorProjectInfo project) => new(FilePathNormalizer.GetNormalizedDirectoryName(project.SerializedFilePath));

@DustinCampbell DustinCampbell Apr 30, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! You found a use for SerializedFilePath! I hope it's accurate. 😄

public static ProjectKey From(Project project)
{
var intermediateOutputPath = FilePathNormalizer.GetNormalizedDirectoryName(project.CompilationOutputInfo.AssemblyPath);
Expand Down
67 changes: 42 additions & 25 deletions ...crosoft.AspNetCore.Razor.LanguageServer.Test/ProjectConfigurationStateSynchronizerTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,19 +28,32 @@ namespace Microsoft.AspNetCore.Razor.LanguageServer;
public class ProjectConfigurationStateSynchronizerTest(ITestOutputHelper testOutput) : LanguageServerTestBase(testOutput)
{
[Fact]
public async Task ProjectConfigurationFileChanged_Removed_UnknownDocumentNoops()
public async Task ProjectConfigurationFileChanged_Removed_UntrackedProject_CallsUpdate()

@davidwengier davidwengier Apr 30, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a change to the logic of the synchronizer itself, where it will call Update, but Update will no-op for a non-existent project, so holistically the behaviour is the same

{
// Arrange
var projectServiceMock = new StrictMock<IRazorProjectService>();

using var synchronizer = GetSynchronizer(projectServiceMock.Object);
var synchronizerAccessor = synchronizer.GetTestAccessor();

var args = new ProjectConfigurationFileChangeEventArgs(
configurationFilePath: "/path/to/project.razor.bin",
kind: RazorFileChangeKind.Removed,
deserializer: StrictMock.Of<IRazorProjectInfoDeserializer>());

var intermediateOutputPath = FilePathNormalizer.GetNormalizedDirectoryName(args.ConfigurationFilePath);
var projectKey = TestProjectKey.Create(intermediateOutputPath);

var projectServiceMock = new StrictMock<IRazorProjectService>();
projectServiceMock
.Setup(p => p.UpdateProjectAsync(
projectKey,
It.IsAny<RazorConfiguration>(),
It.IsAny<string>(),
It.IsAny<string>(),
It.IsAny<ProjectWorkspaceState>(),
It.IsAny<ImmutableArray<DocumentSnapshotHandle>>(),
It.IsAny<CancellationToken>()))
.Returns(Task.CompletedTask);

using var synchronizer = GetSynchronizer(projectServiceMock.Object);
var synchronizerAccessor = synchronizer.GetTestAccessor();

// Act
synchronizer.ProjectConfigurationFileChanged(args);

Expand Down Expand Up @@ -446,30 +459,43 @@ public async Task ProjectConfigurationFileChanged_Changed_CantDeserialize_Resets
}

[Fact]
public async Task ProjectConfigurationFileChanged_Changed_UntrackedProject_Noops()
public async Task ProjectConfigurationFileChanged_Changed_UntrackedProject_CallsUpdate()

@davidwengier davidwengier Apr 30, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, holistically the same

{
// Arrange
var projectService = new StrictMock<IRazorProjectService>();

using var synchronizer = GetSynchronizer(projectService.Object);
var synchronizerAccessor = synchronizer.GetTestAccessor();

var changedArgs = new ProjectConfigurationFileChangeEventArgs(
configurationFilePath: "/path/to/project.razor.bin",
kind: RazorFileChangeKind.Changed,
deserializer: CreateDeserializer(projectInfo: null));

var intermediateOutputPath = FilePathNormalizer.GetNormalizedDirectoryName(changedArgs.ConfigurationFilePath);
var projectKey = TestProjectKey.Create(intermediateOutputPath);

var projectServiceMock = new StrictMock<IRazorProjectService>();
projectServiceMock
.Setup(p => p.UpdateProjectAsync(
projectKey,
It.IsAny<RazorConfiguration>(),
It.IsAny<string>(),
It.IsAny<string>(),
It.IsAny<ProjectWorkspaceState>(),
It.IsAny<ImmutableArray<DocumentSnapshotHandle>>(),
It.IsAny<CancellationToken>()))
.Returns(Task.CompletedTask);

using var synchronizer = GetSynchronizer(projectServiceMock.Object);
var synchronizerAccessor = synchronizer.GetTestAccessor();

// Act
synchronizer.ProjectConfigurationFileChanged(changedArgs);

await synchronizerAccessor.WaitUntilCurrentBatchCompletesAsync();

// Assert
projectService.VerifyAll();
projectServiceMock.VerifyAll();
}

[Fact]
public async Task ProjectConfigurationFileChanged_RemoveThenAdd_OnlyAdds()
public async Task ProjectConfigurationFileChanged_RemoveThenAdd_Updates()
Comment on lines -472 to +498

@davidwengier davidwengier Apr 30, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test didn't do what its name implied it did. It actually verified that if you Add then Change, you get an Add then Update.

The test now does what its name implies, which is actually testing the restored functionality of collapsing removes and adds into a single Update operation.

@DustinCampbell DustinCampbell Apr 30, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! These tests made my eyes cross. 😄

{
// Arrange
var projectInfo = new RazorProjectInfo(
Expand All @@ -484,15 +510,6 @@ public async Task ProjectConfigurationFileChanged_RemoveThenAdd_OnlyAdds()
var projectKey = TestProjectKey.Create(intermediateOutputPath);

var projectServiceMock = new StrictMock<IRazorProjectService>();
projectServiceMock
.Setup(service => service.AddProjectAsync(
projectInfo.FilePath,
intermediateOutputPath,
It.IsAny<RazorConfiguration>(),
projectInfo.RootNamespace,
projectInfo.DisplayName,
It.IsAny<CancellationToken>()))
.ReturnsAsync(projectKey);
projectServiceMock
.Setup(p => p.UpdateProjectAsync(
projectKey,
Expand All @@ -508,12 +525,12 @@ public async Task ProjectConfigurationFileChanged_RemoveThenAdd_OnlyAdds()
var synchronizerAccessor = synchronizer.GetTestAccessor();

var deserializer = CreateDeserializer(projectInfo);
var removedArgs = new ProjectConfigurationFileChangeEventArgs(projectInfo.SerializedFilePath, RazorFileChangeKind.Removed, deserializer);
var addedArgs = new ProjectConfigurationFileChangeEventArgs(projectInfo.SerializedFilePath, RazorFileChangeKind.Added, deserializer);
var changedArgs = new ProjectConfigurationFileChangeEventArgs(projectInfo.SerializedFilePath, RazorFileChangeKind.Changed, deserializer);

// Act
synchronizer.ProjectConfigurationFileChanged(removedArgs);
synchronizer.ProjectConfigurationFileChanged(addedArgs);
synchronizer.ProjectConfigurationFileChanged(changedArgs);

await synchronizerAccessor.WaitUntilCurrentBatchCompletesAsync();

Expand Down