Use work done progress to handle restore server side #81233
Conversation
| /// Manages server initiated work done progress reporting to the client. | ||
| /// See https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#serverInitiatedProgress | ||
| /// </summary> | ||
| class WorkDoneProgressManager(IClientLanguageServerManager clientLanguageServerManager) : ILspService |
There was a problem hiding this comment.
the most interesting part of the change. happy to take feedback here on more ergonomic ways to implement.
here we have to handle sending the initial request to create the progress, sending reports, completing the progress. Additionally, this needs to support either being cancelled by the server or client.
src/LanguageServer/Protocol/Handler/WorkDoneProgress/WorkDoneProgressManager.cs
Show resolved
Hide resolved
|
|
||
| // Tell the client to end the work done progress if the server cancels the request. | ||
| // This needs to not observe the linked cancellation token as it will already be cancelled. | ||
| serverCancellationToken.Register(() => |
There was a problem hiding this comment.
we observe only the server cancellation token here. if the client cancels, there is no need to send this notification as the client already knows it is cancelled.
CyrusNajmabadi
left a comment
CyrusNajmabadi
left a comment
There was a problem hiding this comment.
A little too complex to sign off just yet. Probably want a quick chat to understand more.
...r/Microsoft.CodeAnalysis.LanguageServer/FileBasedPrograms/CanonicalMiscFilesProjectLoader.cs
Outdated
Show resolved
Hide resolved
...ageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectLoader.cs
Show resolved
Hide resolved
| Contract.ThrowIfNull(context.Solution); | ||
| using var progress = BufferedProgress.Create(request.PartialResultToken); | ||
|
|
||
| progress.Report(new RestorePartialResult(LanguageServerResources.Restore, LanguageServerResources.Restore_started)); |
There was a problem hiding this comment.
Why no progress reporting?
There was a problem hiding this comment.
Progress is reported via the work done progress mechanism below instead.
Background
This part of the handler handles manual restore requests from the client. Previously we were using custom client side code to make a streaming request to the server and show progress on the client.
Now the client makes a normal request, and the server enables progress reporting via work done progress.
...erver/Microsoft.CodeAnalysis.LanguageServer/LanguageServer/Handler/Restore/RestoreHandler.cs
Outdated
Show resolved
Hide resolved
...erver/Microsoft.CodeAnalysis.LanguageServer/LanguageServer/Handler/Restore/RestoreHandler.cs
Show resolved
Hide resolved
src/LanguageServer/Protocol/Handler/WorkDoneProgress/WorkDoneProgressManager.cs
Show resolved
Hide resolved
src/LanguageServer/Protocol/Handler/WorkDoneProgress/WorkDoneProgressManager.cs
Show resolved
Hide resolved
src/LanguageServer/Protocol/Handler/WorkDoneProgress/WorkDoneProgressManager.cs
Show resolved
Hide resolved
...erver/Microsoft.CodeAnalysis.LanguageServer/LanguageServer/Handler/Restore/RestoreHandler.cs
Outdated
Show resolved
Hide resolved
| using var progress = await workDoneProgressManager.CreateWorkDoneProgressAsync(reportProgressToClient: enableProgressReporting, cancellationToken); | ||
| // Ensure we're observing cancellation token from the work done progress (to allow client cancellation). | ||
| cancellationToken = progress.CancellationToken; | ||
| return await RestoreCoreAsync(pathsToRestore, progress, dotnetCliHelper, logger, cancellationToken); |
There was a problem hiding this comment.
Is progress.CancellationToken already linked to cancellationToken? Or we don't want to cancel RestoreCoreAsync, when the original cancellationToken parameter value is canceled?
There was a problem hiding this comment.
Is progress.CancellationToken already linked to cancellationToken
Yeah, progress.CancellationToken is linked to the cancellation token and client side token.
src/LanguageServer/Protocol/Handler/WorkDoneProgress/WorkDoneProgressManager.cs
Show resolved
Hide resolved
src/LanguageServer/Protocol/Handler/WorkDoneProgress/WorkDoneProgressManager.cs
Outdated
Show resolved
Hide resolved
|
|
||
| // Perform restore and mock up project restore client handler | ||
| // Perform restore | ||
| ProcessUtilities.Run("dotnet", $"restore --project {projectPath}"); |
There was a problem hiding this comment.
Do you feel the new WorkDoneProgress code is exercised well by the existing tests?
There was a problem hiding this comment.
Restore wasn't really exercised by the tests at all really. We likely need integration tests for this that actually do the full load / restore loop.
3 participants
Followup to the canonical misc changes. This PR has two parts
Move server initiated restore entirely to server
Previously restores initiated by the server had to call out to the client to setup the correct UI, which then called back to the server to run the restore, all using custom LSP requests. This wasn't ideal as it meant extra custom client side code and an extra server -> client -> server loop.
Instead, we can utilize the standard LSP server initiated work done progress API to display progress from the server on the client side. This means server initiated restores no longer need any custom client code, and the server can choose when to display a UI without a client code change.
This did require a bit of complex code to handle either the server or client cancelling the request.
Additionally, we still have custom requests for manually initiated restores from the client.
Hide restore progress for canonical files
Utilizing the above change, I modified the server behavior to not show any progress when restoring the canonical file. This is an internal server operation and does not need to be displayed client side (based on feedback it only causes confusion).
CLient side change - dotnet/vscode-csharp#8780