Skip to content

Remove dispatcher from DocumentVersionCache #9026

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

Merged
merged 3 commits into from
Jul 26, 2023
Merged

Remove dispatcher from DocumentVersionCache #9026

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
02a9faf
Remove, and remove async method
Jul 26, 2023
e9be5f1
Fix locking and unintended infinite recursion
Jul 26, 2023
727f064
Update src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Defaul…
ryzngard Jul 26, 2023
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
126 changes: 63 additions & 63 deletions src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/DefaultDocumentVersionCache.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@
using System;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.Razor;
using Microsoft.CodeAnalysis.Razor.ProjectSystem;

Expand All @@ -16,17 +14,16 @@ internal class DefaultDocumentVersionCache : DocumentVersionCache
internal const int MaxDocumentTrackingCount = 20;

// Internal for testing
internal readonly Dictionary<string, List<DocumentEntry>> DocumentLookup;
private readonly ProjectSnapshotManagerDispatcher _dispatcher;
internal readonly Dictionary<string, List<DocumentEntry>> DocumentLookup_NeedsLock;
private readonly ReadWriterLocker _lock = new();
private ProjectSnapshotManagerBase? _projectSnapshotManager;

private ProjectSnapshotManagerBase ProjectSnapshotManager
=> _projectSnapshotManager ?? throw new InvalidOperationException("ProjectSnapshotManager accessed before Initialized was called.");

public DefaultDocumentVersionCache(ProjectSnapshotManagerDispatcher dispatcher)
public DefaultDocumentVersionCache()
{
_dispatcher = dispatcher ?? throw new ArgumentNullException(nameof(dispatcher));
DocumentLookup = new Dictionary<string, List<DocumentEntry>>(FilePathComparer.Instance);
DocumentLookup_NeedsLock = new(FilePathComparer.Instance);
}

public override void TrackDocumentVersion(IDocumentSnapshot documentSnapshot, int version)
Expand All @@ -36,27 +33,34 @@ public override void TrackDocumentVersion(IDocumentSnapshot documentSnapshot, in
throw new ArgumentNullException(nameof(documentSnapshot));
}

_dispatcher.AssertDispatcherThread();

var filePath = documentSnapshot.FilePath.AssumeNotNull();
using var upgradeableReadLock = _lock.EnterUpgradeAbleReadLock();
TrackDocumentVersion(documentSnapshot, version, filePath, upgradeableReadLock);
}

if (!DocumentLookup.TryGetValue(filePath, out var documentEntries))
private void TrackDocumentVersion(IDocumentSnapshot documentSnapshot, int version, string filePath, ReadWriterLocker.UpgradeableReadLock upgradeableReadLock)
{
// Need to ensure the write lock covers all uses of documentEntries, not just DocumentLookup
using (upgradeableReadLock.EnterWriteLock())
Copy link
Contributor

@allisonchou allisonchou Jul 26, 2023

Choose a reason for hiding this comment

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

This might just be me not understanding locks properly, but is there a reason why we're using using () syntax instead of using var _ syntax as we do in most other parts of this file? Is the intention to just be more explicit about the scope of the write lock?

Copy link
Contributor Author

@ryzngard ryzngard Jul 26, 2023

Choose a reason for hiding this comment

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

For write locks we're being a bit more explicit about where it is held, since they block all other operations.

{
documentEntries = new List<DocumentEntry>();
DocumentLookup[filePath] = documentEntries;
}
if (!DocumentLookup_NeedsLock.TryGetValue(filePath, out var documentEntries))
{
documentEntries = new List<DocumentEntry>();
DocumentLookup_NeedsLock[filePath] = documentEntries;
}

if (documentEntries.Count == MaxDocumentTrackingCount)
{
// Clear the oldest document entry
if (documentEntries.Count == MaxDocumentTrackingCount)
{
// Clear the oldest document entry

// With this approach we'll slowly leak memory as new documents are added to the system. We don't clear up
// document file paths where where all of the corresponding entries are expired.
documentEntries.RemoveAt(0);
}
// With this approach we'll slowly leak memory as new documents are added to the system. We don't clear up
// document file paths where where all of the corresponding entries are expired.
documentEntries.RemoveAt(0);
}

var entry = new DocumentEntry(documentSnapshot, version);
documentEntries.Add(entry);
var entry = new DocumentEntry(documentSnapshot, version);
documentEntries.Add(entry);
}
}

public override bool TryGetDocumentVersion(IDocumentSnapshot documentSnapshot, [NotNullWhen(true)] out int? version)
Expand All @@ -66,11 +70,10 @@ public override bool TryGetDocumentVersion(IDocumentSnapshot documentSnapshot, [
throw new ArgumentNullException(nameof(documentSnapshot));
}

_dispatcher.AssertDispatcherThread();

var filePath = documentSnapshot.FilePath.AssumeNotNull();
using var _ = _lock.EnterReadLock();

if (!DocumentLookup.TryGetValue(filePath, out var documentEntries))
if (!DocumentLookup_NeedsLock.TryGetValue(filePath, out var documentEntries))
{
version = null;
return false;
Expand Down Expand Up @@ -98,22 +101,6 @@ public override bool TryGetDocumentVersion(IDocumentSnapshot documentSnapshot, [
return true;
}

public override Task<int?> TryGetDocumentVersionAsync(IDocumentSnapshot documentSnapshot, CancellationToken cancellationToken)
{
if (documentSnapshot is null)
{
throw new ArgumentNullException(nameof(documentSnapshot));
}

return _dispatcher.RunOnDispatcherThreadAsync(
() =>
{
TryGetDocumentVersion(documentSnapshot, out var version);
return version;
},
cancellationToken);
}

public override void Initialize(ProjectSnapshotManagerBase projectManager)
{
if (projectManager is null)
Expand All @@ -133,18 +120,21 @@ private void ProjectSnapshotManager_Changed(object? sender, ProjectChangeEventAr
return;
}

_dispatcher.AssertDispatcherThread();
var upgradeableLock = _lock.EnterUpgradeAbleReadLock();

switch (args.Kind)
{
case ProjectChangeKind.DocumentChanged:
var documentFilePath = args.DocumentFilePath!;
if (DocumentLookup.ContainsKey(documentFilePath) &&
!ProjectSnapshotManager.IsDocumentOpen(documentFilePath))
{
// Document closed, evict entry.
DocumentLookup.Remove(documentFilePath);
}
var documentFilePath = args.DocumentFilePath!;
if (DocumentLookup_NeedsLock.ContainsKey(documentFilePath) &&
!ProjectSnapshotManager.IsDocumentOpen(documentFilePath))
{
using (upgradeableLock.EnterWriteLock())
{
// Document closed, evict entry.
DocumentLookup_NeedsLock.Remove(documentFilePath);
}
}

break;
}
Expand All @@ -163,27 +153,22 @@ private void ProjectSnapshotManager_Changed(object? sender, ProjectChangeEventAr
return;
}

CaptureProjectDocumentsAsLatest(project);
CaptureProjectDocumentsAsLatest(project, upgradeableLock);
}

// Internal for testing
internal void MarkAsLatestVersion(IDocumentSnapshot document)
{
var filePath = document.FilePath.AssumeNotNull();

if (!TryGetLatestVersionFromPath(filePath, out var latestVersion))
{
return;
}

// Update our internal tracking state to track the changed document as the latest document.
TrackDocumentVersion(document, latestVersion.Value);
using var upgradeableLock = _lock.EnterUpgradeAbleReadLock();
MarkAsLatestVersion(document, upgradeableLock);
}

// Internal for testing
internal bool TryGetLatestVersionFromPath(string filePath, [NotNullWhen(true)] out int? version)
{
if (!DocumentLookup.TryGetValue(filePath, out var documentEntries))
using var _ = _lock.EnterReadLock();

if (!DocumentLookup_NeedsLock.TryGetValue(filePath, out var documentEntries))
{
version = null;
return false;
Expand All @@ -195,18 +180,33 @@ internal bool TryGetLatestVersionFromPath(string filePath, [NotNullWhen(true)] o
return true;
}

private void CaptureProjectDocumentsAsLatest(IProjectSnapshot projectSnapshot)
private void CaptureProjectDocumentsAsLatest(IProjectSnapshot projectSnapshot, ReadWriterLocker.UpgradeableReadLock upgradeableReadLock)
{
foreach (var documentPath in projectSnapshot.DocumentFilePaths)
{
if (DocumentLookup.ContainsKey(documentPath) &&
if (DocumentLookup_NeedsLock.ContainsKey(documentPath) &&
projectSnapshot.GetDocument(documentPath) is { } document)
{
MarkAsLatestVersion(document);
MarkAsLatestVersion(document, upgradeableReadLock);
}
}
}

private void MarkAsLatestVersion(IDocumentSnapshot document, ReadWriterLocker.UpgradeableReadLock upgradeableReadLock)
{
var filePath = document.FilePath.AssumeNotNull();

if (!DocumentLookup_NeedsLock.TryGetValue(filePath, out var documentEntries))
{
return;
}

var latestEntry = documentEntries[^1];

// Update our internal tracking state to track the changed document as the latest document.
TrackDocumentVersion(document, latestEntry.Version, document.FilePath.AssumeNotNull(), upgradeableReadLock);
}

internal class DocumentEntry
{
public DocumentEntry(IDocumentSnapshot document, int version)
Expand Down
5 changes: 0 additions & 5 deletions src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/DocumentVersionCache.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,12 @@
// Licensed under the MIT license. See License.txt in the project root for license information.

using System.Diagnostics.CodeAnalysis;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.Razor.ProjectSystem;

namespace Microsoft.AspNetCore.Razor.LanguageServer;

internal abstract class DocumentVersionCache : ProjectSnapshotChangeTrigger
{
public abstract bool TryGetDocumentVersion(IDocumentSnapshot documentSnapshot, [NotNullWhen(true)] out int? version);

public abstract Task<int?> TryGetDocumentVersionAsync(IDocumentSnapshot documentSnapshot, CancellationToken cancellationToken);

public abstract void TrackDocumentVersion(IDocumentSnapshot documentSnapshot, int version);
}
6 changes: 2 additions & 4 deletions src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/HtmlFormatter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,7 @@ public async Task<TextEdit[]> FormatAsync(
throw new ArgumentNullException(nameof(context));
}

var documentVersion = await _documentVersionCache.TryGetDocumentVersionAsync(context.OriginalSnapshot, cancellationToken).ConfigureAwait(false);
if (documentVersion is null)
if (!_documentVersionCache.TryGetDocumentVersion(context.OriginalSnapshot, out var documentVersion))
{
return Array.Empty<TextEdit>();
}
Expand All @@ -63,8 +62,7 @@ public async Task<TextEdit[]> FormatOnTypeAsync(
FormattingContext context,
CancellationToken cancellationToken)
{
var documentVersion = await _documentVersionCache.TryGetDocumentVersionAsync(context.OriginalSnapshot, cancellationToken).ConfigureAwait(false);
if (documentVersion == null)
if (!_documentVersionCache.TryGetDocumentVersion(context.OriginalSnapshot, out var documentVersion))
{
return Array.Empty<TextEdit>();
}
Expand Down
2 changes: 1 addition & 1 deletion .../test/Microsoft.AspNetCore.Razor.LanguageServer.Test/DefaultDocumentContextFactoryTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ public class DefaultDocumentContextFactoryTest : LanguageServerTestBase
public DefaultDocumentContextFactoryTest(ITestOutputHelper testOutput)
: base(testOutput)
{
_documentVersionCache = new DefaultDocumentVersionCache(Dispatcher);
_documentVersionCache = new DefaultDocumentVersionCache();
}

[Fact]
Expand Down
28 changes: 14 additions & 14 deletions ...or/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/DefaultDocumentVersionCacheTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ public DefaultDocumentVersionCacheTest(ITestOutputHelper testOutput)
public void MarkAsLatestVersion_UntrackedDocument_Noops()
{
// Arrange
var documentVersionCache = new DefaultDocumentVersionCache(LegacyDispatcher);
var documentVersionCache = new DefaultDocumentVersionCache();
var document = TestDocumentSnapshot.Create("C:/file.cshtml");
documentVersionCache.TrackDocumentVersion(document, 123);
var untrackedDocument = TestDocumentSnapshot.Create("C:/other.cshtml");
Expand All @@ -39,7 +39,7 @@ public void MarkAsLatestVersion_UntrackedDocument_Noops()
public void MarkAsLatestVersion_KnownDocument_TracksNewDocumentAsLatest()
{
// Arrange
var documentVersionCache = new DefaultDocumentVersionCache(LegacyDispatcher);
var documentVersionCache = new DefaultDocumentVersionCache();
var documentInitial = TestDocumentSnapshot.Create("C:/file.cshtml");
documentVersionCache.TrackDocumentVersion(documentInitial, 123);
var documentLatest = TestDocumentSnapshot.Create(documentInitial.FilePath);
Expand All @@ -56,7 +56,7 @@ public void MarkAsLatestVersion_KnownDocument_TracksNewDocumentAsLatest()
public void TryGetLatestVersionFromPath_TrackedDocument_ReturnsTrue()
{
// Arrange
var documentVersionCache = new DefaultDocumentVersionCache(LegacyDispatcher);
var documentVersionCache = new DefaultDocumentVersionCache();
var filePath = "C:/file.cshtml";
var document1 = TestDocumentSnapshot.Create(filePath);
var document2 = TestDocumentSnapshot.Create(filePath);
Expand All @@ -75,7 +75,7 @@ public void TryGetLatestVersionFromPath_TrackedDocument_ReturnsTrue()
public void TryGetLatestVersionFromPath_UntrackedDocument_ReturnsFalse()
{
// Arrange
var documentVersionCache = new DefaultDocumentVersionCache(LegacyDispatcher);
var documentVersionCache = new DefaultDocumentVersionCache();

// Act
var result = documentVersionCache.TryGetLatestVersionFromPath("C:/file.cshtml", out var version);
Expand All @@ -89,7 +89,7 @@ public void TryGetLatestVersionFromPath_UntrackedDocument_ReturnsFalse()
public void ProjectSnapshotManager_Changed_DocumentRemoved_DoesNotEvictDocument()
{
// Arrange
var documentVersionCache = new DefaultDocumentVersionCache(LegacyDispatcher);
var documentVersionCache = new DefaultDocumentVersionCache();
var projectSnapshotManager = TestProjectSnapshotManager.Create(ErrorReporter);
projectSnapshotManager.AllowNotifyListeners = true;
documentVersionCache.Initialize(projectSnapshotManager);
Expand Down Expand Up @@ -119,7 +119,7 @@ public void ProjectSnapshotManager_Changed_DocumentRemoved_DoesNotEvictDocument(
public void ProjectSnapshotManager_Changed_OpenDocumentRemoved_DoesNotEvictDocument()
{
// Arrange
var documentVersionCache = new DefaultDocumentVersionCache(LegacyDispatcher);
var documentVersionCache = new DefaultDocumentVersionCache();
var projectSnapshotManager = TestProjectSnapshotManager.Create(ErrorReporter);
projectSnapshotManager.AllowNotifyListeners = true;
documentVersionCache.Initialize(projectSnapshotManager);
Expand Down Expand Up @@ -151,7 +151,7 @@ public void ProjectSnapshotManager_Changed_OpenDocumentRemoved_DoesNotEvictDocum
public void ProjectSnapshotManager_Changed_DocumentClosed_EvictsDocument()
{
// Arrange
var documentVersionCache = new DefaultDocumentVersionCache(LegacyDispatcher);
var documentVersionCache = new DefaultDocumentVersionCache();
var projectSnapshotManager = TestProjectSnapshotManager.Create(ErrorReporter);
projectSnapshotManager.AllowNotifyListeners = true;
documentVersionCache.Initialize(projectSnapshotManager);
Expand Down Expand Up @@ -183,14 +183,14 @@ public void ProjectSnapshotManager_Changed_DocumentClosed_EvictsDocument()
public void TrackDocumentVersion_AddsFirstEntry()
{
// Arrange
var documentVersionCache = new DefaultDocumentVersionCache(LegacyDispatcher);
var documentVersionCache = new DefaultDocumentVersionCache();
var document = TestDocumentSnapshot.Create("C:/file.cshtml");

// Act
documentVersionCache.TrackDocumentVersion(document, 1337);

// Assert
var kvp = Assert.Single(documentVersionCache.DocumentLookup);
var kvp = Assert.Single(documentVersionCache.DocumentLookup_NeedsLock);
Assert.Equal(document.FilePath, kvp.Key);
var entry = Assert.Single(kvp.Value);
Assert.True(entry.Document.TryGetTarget(out var actualDocument));
Expand All @@ -202,7 +202,7 @@ public void TrackDocumentVersion_AddsFirstEntry()
public void TrackDocumentVersion_EvictsOldEntries()
{
// Arrange
var documentVersionCache = new DefaultDocumentVersionCache(LegacyDispatcher);
var documentVersionCache = new DefaultDocumentVersionCache();
var document = TestDocumentSnapshot.Create("C:/file.cshtml");

for (var i = 0; i < DefaultDocumentVersionCache.MaxDocumentTrackingCount; i++)
Expand All @@ -214,7 +214,7 @@ public void TrackDocumentVersion_EvictsOldEntries()
documentVersionCache.TrackDocumentVersion(document, 1337);

// Assert
var kvp = Assert.Single(documentVersionCache.DocumentLookup);
var kvp = Assert.Single(documentVersionCache.DocumentLookup_NeedsLock);
Assert.Equal(DefaultDocumentVersionCache.MaxDocumentTrackingCount, kvp.Value.Count);
Assert.Equal(1337, kvp.Value.Last().Version);
}
Expand All @@ -223,7 +223,7 @@ public void TrackDocumentVersion_EvictsOldEntries()
public void TryGetDocumentVersion_UntrackedDocumentPath_ReturnsFalse()
{
// Arrange
var documentVersionCache = new DefaultDocumentVersionCache(LegacyDispatcher);
var documentVersionCache = new DefaultDocumentVersionCache();
var document = TestDocumentSnapshot.Create("C:/file.cshtml");

// Act
Expand All @@ -238,7 +238,7 @@ public void TryGetDocumentVersion_UntrackedDocumentPath_ReturnsFalse()
public void TryGetDocumentVersion_EvictedDocument_ReturnsFalse()
{
// Arrange
var documentVersionCache = new DefaultDocumentVersionCache(LegacyDispatcher);
var documentVersionCache = new DefaultDocumentVersionCache();
var document = TestDocumentSnapshot.Create("C:/file.cshtml");
var evictedDocument = TestDocumentSnapshot.Create(document.FilePath);
documentVersionCache.TrackDocumentVersion(document, 1337);
Expand All @@ -255,7 +255,7 @@ public void TryGetDocumentVersion_EvictedDocument_ReturnsFalse()
public void TryGetDocumentVersion_KnownDocument_ReturnsTrue()
{
// Arrange
var documentVersionCache = new DefaultDocumentVersionCache(LegacyDispatcher);
var documentVersionCache = new DefaultDocumentVersionCache();
var document = TestDocumentSnapshot.Create("C:/file.cshtml");
documentVersionCache.TrackDocumentVersion(document, 1337);

Expand Down
2 changes: 1 addition & 1 deletion ...t/Microsoft.AspNetCore.Razor.LanguageServer.Test/Formatting/TestRazorFormattingService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public static async Task<IRazorFormattingService> CreateWithFullSupportAsync(
var mappingService = new RazorDocumentMappingService(TestLanguageServerFeatureOptions.Instance, new TestDocumentContextFactory(), loggerFactory);

var dispatcher = new LSPProjectSnapshotManagerDispatcher(loggerFactory);
var versionCache = new DefaultDocumentVersionCache(dispatcher);
var versionCache = new DefaultDocumentVersionCache();
if (documentSnapshot is not null)
{
await dispatcher.RunOnDispatcherThreadAsync(() =>
Expand Down
Loading