Refresh diagnostics on project changes #10964

ryzngard · 2024-10-03T23:21:59Z

This notifies a client to refresh diagnostics when we experience project snapshot changes (except for document changed). This should help for cases where the project engine is updating but doesn't have all taghelper information yet.

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/WorkspaceDiagnosticsRefresher.cs

davidwengier

I see now this is how the semantic tokens refresher works, so I'm going to approve, but I think there is an argument to be made to move both impls to the AsyncBatchingWorkQueue as its a bit more battle hardened. Could even do a common base class for both that does most of the work, and just has an abstract method to check client capabilites, and a property for which LSP method to send.

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/WorkspaceDiagnosticsRefresher.cs

src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/WorkspaceDiagnosticRefreshTest.cs

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/WorkspaceDiagnosticsRefresher.cs

DustinCampbell · 2024-10-04T20:51:00Z

...azor/src/Microsoft.AspNetCore.Razor.LanguageServer/WorkspaceSemanticTokensRefreshNotifier.cs

-    private bool? _supportsRefresh;
-    private bool _waitingToRefresh;
-    private Task _refreshTask = Task.CompletedTask;
+    private readonly AsyncBatchingWorkQueue _queue;


Amusingly, I had used an AsyncBatchingWorkQueue in #10140 and removed it based on feedback.

well I'm bringing it back! :P

...azor/src/Microsoft.AspNetCore.Razor.LanguageServer/WorkspaceSemanticTokensRefreshNotifier.cs

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/WorkspaceDiagnosticsRefresher.cs

DustinCampbell · 2024-10-04T21:05:49Z

src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/WorkspaceDiagnosticRefreshTest.cs

+        var clientConnection = new StrictMock<IClientConnection>();
+        clientConnection
+            .Setup(c => c.SendNotificationAsync(Methods.WorkspaceDiagnosticRefreshName, It.IsAny<CancellationToken>()))
+            .Returns(Task.CompletedTask)


Does ReturnsAsync not work here and below?

Idk tbh. What's the difference?

It looks like not. ReturnsAsync doesn't have a way to do a Task, it requires Task<T>

Ah, I think I'm remembering a Moq extension I had added in a branch to address that.

Idk tbh. What's the difference?

ReturnsAsync(...) is just convenient for returning Task<T> and ValueTask<T> in mocks.

DustinCampbell · 2024-10-04T21:13:25Z

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/WorkspaceDiagnosticsRefresher.cs

+
+        _disposeTokenSource.Cancel();
+        _disposeTokenSource.Dispose();
+        _projectSnapshotManager.Changed -= ProjectSnapshotManager_Changed;


💭 Consider moving this above _disposeTokenSource.Cancel() so that no more work can be added before cancelling. If IProjectSnapshotManager.Updated fires while the AsyncBatchingWorkQueue is being cancelled, it might try to add work, which can result in an OperationCancelledException. Not a huge deal, but definitely unneeded.

I didn't know adding work threw if the ABWQ cancellation happened. This is good to know , thank you!

It's a very tight race, but it's possible to land here:

razor/src/Razor/src/Microsoft.AspNetCore.Razor.ProjectEngineHost/Utilities/AsyncBatchingWorkQueue`2.cs

Line 205 in 9c97001

_entireQueueCancellationToken.ThrowIfCancellationRequested();

DustinCampbell · 2024-10-04T21:14:46Z

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/WorkspaceDiagnosticsRefresher.cs

+            return;
+        }
+
+        if ( _disposeTokenSource.IsCancellationRequested)


nit: extra space between ( and _. Or, if you decide to take my feedback to adjust the Dispose() method, this check isn't needed at all! 😄

I'm on a laptop today and it shows 😅

DustinCampbell · 2024-10-04T21:15:54Z

src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/WorkspaceDiagnosticRefreshTest.cs

+                updater.CreateAndAddProject("C:/path/to/project.csproj");
+            });
+
+        await testAccessor.WaitForRefreshAsync();


Since publisher is disposed above, will this ever complete?

I believe so. The underlying ABWQ method doesn't seem to check for cancellation, it just returns the task. That task is completed when this hits

DustinCampbell · 2024-10-04T21:19:57Z

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/WorkspaceDiagnosticsRefresher.cs

+
+        public Task WaitForRefreshAsync()
+        {
+            return _instance._queue.WaitUntilCurrentBatchCompletesAsync();


FWIW, there might be a hang in tests where WaitForRefreshAsync() is called after the WorkspaceDiagnosticRefresher is disposed. I'm not 100% sure though. I'm trying to "psychically debug" this. 😄

Is it better to check disposal here and return a Task.CompletedTask?

I quick fix might be something like...

if (_instance._disposeTokenSource.IsCancellationRequested) { return Task.CompletedTask; }

Is it better to check disposal here and return a Task.CompletedTask?

LOL. I didn't refresh quickly enough to see your comment. 🤣

davidwengier

Love how small the class is now.

I still think its worth considering a common base class to abstract away most of the code, but I understand if it's beyond the scope of this PR, or if I'm missing some benefit of the semantic notifier being split across two classes.

ryzngard added 2 commits October 3, 2024 15:19

Add imple

aa60e02

Rename and add tests

dd8d9e2

ryzngard requested a review from a team as a code owner October 3, 2024 23:22

davidwengier reviewed Oct 4, 2024

View reviewed changes

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/WorkspaceDiagnosticsRefresher.cs Outdated Show resolved Hide resolved

davidwengier approved these changes Oct 4, 2024

View reviewed changes