Fix workspace pull diagnostics #66886
Conversation
dibarbet
force-pushed
the
workspace_diagnostics_poll
branch
3 times, most recently
from
3a80693
to
1d8f137
Compare
| // have happened while this request was running or might happen after. | ||
| // Only if there are no changes between now and the next request completing, we will hold the connection open while we poll for changes. | ||
| _lspChanged = false; | ||
| return; |
There was a problem hiding this comment.
nit: could just use an InterlockedExchange here.
| await Task.Delay(TimeSpan.FromMilliseconds(100)) | ||
| .ContinueWith(async (_) => await WaitForChangesAsync().ConfigureAwait(false), TaskScheduler.Default).ConfigureAwait(false); |
There was a problem hiding this comment.
| await Task.Delay(TimeSpan.FromMilliseconds(100)) | |
| .ContinueWith(async (_) => await WaitForChangesAsync().ConfigureAwait(false), TaskScheduler.Default).ConfigureAwait(false); | |
| await Task.Delay(TimeSpan.FromMilliseconds(100)).ConfigureAwait(false); | |
| await WaitForChangesAsync().ConfigureAwait(false); |
?
There was a problem hiding this comment.
ah, i know why i don't like this. just make this an actual loop in code, instead of a function that calls itself :)
dibarbet
force-pushed
the
workspace_diagnostics_poll
branch
2 times, most recently
from
10dea9d
to
d9beb4d
Compare
.../LanguageServer/Protocol/Handler/Diagnostics/Public/PublicWorkspacePullDiagnosticsHandler.cs
Show resolved
Hide resolved
.../LanguageServer/Protocol/Handler/Diagnostics/Public/PublicWorkspacePullDiagnosticsHandler.cs
Show resolved
Hide resolved
.../LanguageServer/Protocol/Handler/Diagnostics/Public/PublicWorkspacePullDiagnosticsHandler.cs
Show resolved
Hide resolved
src/Features/LanguageServer/Protocol/Handler/Diagnostics/AbstractPullDiagnosticHandler.cs
Outdated
Show resolved
Hide resolved
server with requests
dibarbet
force-pushed
the
workspace_diagnostics_poll
branch
from
d9beb4d
to
2d88502
Compare
| Interlocked.Exchange(ref _lspChanged, 1); | ||
| } | ||
|
|
||
| protected override async Task WaitForChangesAsync(RequestContext context) |
There was a problem hiding this comment.
💡 Consider adding another way to exit this (some sort of shutdown cancellation token)
There was a problem hiding this comment.
🤦 ... yes this should absolutely use the cancellation token passed in by the queue. that'll take care of either request cancellation / shutdown. fixed!
dibarbet
changed the title
| await testLspServer.WaitForDiagnosticsAsync(); | ||
|
|
||
| BufferedProgress<WorkspaceDiagnosticPartialReport>? progress = useProgress ? BufferedProgress.Create<WorkspaceDiagnosticPartialReport>(null) : null; | ||
| var diagnosticsTask = testLspServer.ExecuteRequestAsync<WorkspaceDiagnosticParams, WorkspaceDiagnosticReport?>( |
There was a problem hiding this comment.
hide whitespace changes for this
| @@ -21,14 +22,32 @@ namespace Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics.Public; | |||
| using WorkspaceDiagnosticPartialReport = SumType<WorkspaceDiagnosticReport, WorkspaceDiagnosticReportPartialResult>; | |||
|
|
|||
| [Method(Methods.WorkspaceDiagnosticName)] | |||
| internal class PublicWorkspacePullDiagnosticsHandler : AbstractPullDiagnosticHandler<WorkspaceDiagnosticParams, WorkspaceDiagnosticPartialReport, WorkspaceDiagnosticReport?> | |||
| internal class PublicWorkspacePullDiagnosticsHandler : AbstractPullDiagnosticHandler<WorkspaceDiagnosticParams, WorkspaceDiagnosticPartialReport, WorkspaceDiagnosticReport?>, IDisposable | |||
There was a problem hiding this comment.
curious. why only do this in the public workspace pull side? is the non-public side not affected?
There was a problem hiding this comment.
Right, the client polls in the other version.
| { | ||
| _workspaceManager.LspTextChanged -= OnLspTextChanged; | ||
| _workspaceRegistrationService.LspSolutionChanged -= OnLspSolutionChanged; | ||
| } |
There was a problem hiding this comment.
can you please put Dispose right after constructor? i find it much easier to reason about that way. :)
| @@ -87,6 +87,8 @@ internal class LspWorkspaceManager : IDocumentChangeTracker, ILspService | |||
| _lspWorkspaceRegistrationService = lspWorkspaceRegistrationService; | |||
| } | |||
|
|
|||
| public EventHandler<EventArgs>? LspTextChanged; | |||
| _workspaceRegistrationService.LspSolutionChanged += OnLspSolutionChanged; | ||
|
|
||
| _workspaceManager = workspaceManager; | ||
| _workspaceManager.LspTextChanged += OnLspTextChanged; |
There was a problem hiding this comment.
nit: why do we listen for text-changed if we're alrady listening to solution changed? the former seems unnecessary. would that help simplify?
There was a problem hiding this comment.
LspSolutionChange means the backing workspace solution changed, it doesn't include changes for open documents (because we don't create a solution on LSP text changes, only when a request needing a solution comes in).
ghost
added this to the 
Update our public spec implementation of workspace pull diagnostics to hold the request open when nothing changes to avoid the client spamming the server with requests.