Make RazorProjectService operations atomic #10322
Conversation
This change updates SnapshotResolver with an InitializeAsync method that is called when the LSP "initialized" endpoint is handled. When initialized, the SnapshotResolver adds the misc project to the ProjectSnapshotManager. Tests have been updated to call InitializeAsync whenever a SnapshotResolver is created.
Now that the misc project is guaranteed to be in the project snapshot manager, the operation to retrieve the misc project can be made synchronous.
This extension method no longer has awaits and can be made synchronous. In addition, the cancellation token can be removed.
Now that ISnapshotResolver is synchronous, there's no reason for IDocumentContextFactory to be synchronous.
This method now runs synchronously.
The RazorProjectService APIs (e.g. AddDocumentAsync, UpdateProjectAsync, etc.) were chunked into multiple tasks and awaits. That means that other updates to the ProjectSnapshotManager could be interleaved with them. This change ensures that the operations are atomic (as far as the ProjectSnapshotManager is concerned) by running each within a call to ProjectSnapshotManager.UpdateAsync(...). Because of other changes to make CPU-bound async work synchronous the operations themselves can be made synchronous.
....AspNetCore.Razor.LanguageServer.Test/Completion/LegacyRazorCompletionResolveEndpointTest.cs
Show resolved
Hide resolved
davidwengier
left a comment
davidwengier
left a comment
There was a problem hiding this comment.
There were quite a few methods that could now get the proper Try pattern change, but I gave up commenting because I don't feel strongly enough about it.. and you already did two of them anyway :)
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/RazorInitializedEndpoint.cs
Outdated
Show resolved
Hide resolved
Easy enough for a follow up sometime. Note that integration tests passed on this PR. Are we sure that ProjectConfigurationStateSynchronizer change is needed? |
Personally I'm not a fan of the way it currently works, where it resets the project then re-adds it. I'm totally fine if that gets fixed to be smarter, while still maintaining the AsyncBatchingWorkQueue, rather than just reverting the change entirely. |
Yep, thats why I didn't comment. No need to bog down this PR with that |
I ended up updating several of the "Try*" methods anyway. |
| internal interface IOnInitialized | ||
| { | ||
| Task OnInitializedAsync(VSInternalClientCapabilities clientCapabilities, CancellationToken cancellationToken); | ||
| Task InitializeAsync(ILspServices services, CancellationToken cancellationToken); |
There was a problem hiding this comment.
I have no issue with getting rid of the "On", but "initialize" and "initialized" are both LSP concepts, and I think it could be confusing not to be clear about this being past tense. They're essentially before and after server initialization.
There was a problem hiding this comment.
I wasn't thinking of it that way. In my mind, this is just a mechanism for services to be initialized. In that sense, the notion that service initialization is tied to LSP initialization is really an implementation detail, and could potentially even change.
There was a problem hiding this comment.
I think concepts are being mixed here. I'm not sure all of this initialization is the same. For example, IClientConnection's IOnInitialized implementation is intended to ensure that it doesn't make any requests until everything is initialized. However, it might now be initialized before other services that implement IOnInitialized, even though it really wants to be last.
I'm not sure that IOnInitialized is the "right" way to do this. It's feeling clunky to me, like we're mixing in LSP concepts that shouldn't be mixed in. IMO, there should be a standard way for services to perform asynchronous initialization just like there is for disposal, and it seems like this interface is locked up in the specific semantics of how LSP initialization works.
There was a problem hiding this comment.
Happy to talk about this further, as I'm not sure I agree, or probably more correctly, fully understand your perspective. :)
There was a problem hiding this comment.
For sure! I ended up changing it to match your preference, so we can chat about it at your leisure.
4 participants
The work I did to make all of the public entry points on
RazorProjectServiceasync did so by breaking up the operations in multiple tasks. The operations themselves can't interleave because a semaphore gates just one operation at a time. However, the updates to theProjectSnapshotManagermade by each operation can interleave with other updates to theProjectSnapshotManager.This change ensures that all of the
RazorProjectServiceoperations are atomic WRT to theProjectSnapshotMangerby running each operation within a call toProjectSnapshotManager.UpdateAsync(...). In addition, I've taken a suggestion from @ryzngard to change when the miscellaneous files project is added to theProjectSnapshotManagerin the language server. Before this change, the misc files project would be added by the first call toISnapshotResolver.GetMiscellaneousProjectAsync(...). Now, it's added duringSnapshotResolverinitialization. This allowsGetMiscellaneousProjectAsync(...)to become a synchronous method, which has the positive effect allowing many of the downstream callers to be synchronous as well. Ultimately, this allows all of theRazorProjectServiceoperations to be implemented synchronously, which helps ensure that they are actually atomic.