[LSP] Fix race in document change tracker when used in non-mutating requests. #57317
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…cked documents in a non-mutating handler would change depending on text sync requests that were ordered after
dibarbet
added
Area-IDE
LSP
src/Features/LanguageServer/Protocol/Handler/RequestExecutionQueue.DocumentChangeTracker.cs
Show resolved
Hide resolved
| @@ -126,10 +136,10 @@ private void OnWorkspaceRegistered(object? sender, LspWorkspaceRegisteredEventAr | |||
| private void OnWorkspaceChanged(object? sender, WorkspaceChangeEventArgs e) | |||
| { | |||
| var workspace = e.NewSolution.Workspace; | |||
| if (e.Kind is WorkspaceChangeKind.DocumentChanged or WorkspaceChangeKind.AdditionalDocumentChanged or WorkspaceChangeKind.AnalyzerConfigDocumentChanged) | |||
There was a problem hiding this comment.
This was pointed about by Jason on the last PR. I removed them for a couple reasons
- Accessing additional / analyzer docs requires a different API than GetRequiredDocument, so the old code would have thrown on a change to them.
- We currently don't hear about changes to additional/analyzer documents via LSP, so they will always be workspace changes.
At some point if we want to support LSP for those kinds of documents, we'll have to do more changes than the ones here.
src/Features/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs
Outdated
Show resolved
Hide resolved
CyrusNajmabadi
requested changes
Oct 22, 2021
This was referenced Oct 22, 2021
src/Features/LanguageServer/Protocol/Handler/RequestExecutionQueue.DocumentChangeTracker.cs
Outdated
Show resolved
Hide resolved
src/Features/LanguageServer/Protocol/Handler/RequestExecutionQueue.DocumentChangeTracker.cs
Outdated
Show resolved
Hide resolved
src/Features/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs
Outdated
Show resolved
Hide resolved
src/Features/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs
Outdated
Show resolved
Hide resolved
src/Features/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs
Outdated
Show resolved
Hide resolved
src/Features/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs
Outdated
Show resolved
Hide resolved
CyrusNajmabadi
requested changes
Oct 25, 2021
There was a problem hiding this comment.
nits can be ignored. but there still is one read of the field that is done inconsistently. it needs to be doc'ed, or made consistent, or other pure reads need to follow that pattern.
imo, teh simplest and most consistent is to be intentional about the lock everywhere to ensure absolutely clear exclusive semantics over it in all circumstances.
…bet/roslyn into fix_document_tracking_race
| _workspaceToLspSolution[workspace] = null; | ||
| } | ||
|
|
||
| static bool IsDocumentTrackedByLsp(DocumentId changedDocumentId, Solution newWorkspaceSolution, IDocumentChangeTracker documentChangeTracker) | ||
| bool IsDocumentTrackedByLsp(DocumentId changedDocumentId, Solution newWorkspaceSolution, ImmutableDictionary<Uri, SourceText> trackedDocuments) |
There was a problem hiding this comment.
nit. called in one place. pretty trivial. could inline.
CyrusNajmabadi
approved these changes
Oct 25, 2021
|
@jinujoseph for m2 approval - a followup with a few fixes for #57140 which should go into p1 since the previous PR went into p1 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, the NonMutatingDocumentChangeTracker had a reference to the mutating tracker to get tracked documents. However the mutating tracker was definitely not threadsafe and would not accurately reflect the tracked documents based on the request order in the queue (later lsp open requests would be seen as 'tracked' by requests prior to the open since the handler was just looking at current state).
This modifies the change tracking a bit. Now LSpWorkspaceManager implements IDocumentChangeTracker. Each request context then gets a snapshot of the LSP tracked text when the request is de-queued (serially).
Also addresses a couple small bits of feedback from @jasonmalinowski on the PR.
Resolves #57276