[LSP] Fix race in document change tracker when used in non-mutating requests.#57317
Conversation
…cked documents in a non-mutating handler would change depending on text sync requests that were ordered after
src/Features/LanguageServer/Protocol/Handler/RequestExecutionQueue.DocumentChangeTracker.cs
Show resolved
Hide resolved
| 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
left a comment
There was a problem hiding this comment.
the race condition pointi made needs an answer. either it needs a fix, or it needs an explanation for why i'm wrong :)
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
left a comment
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
| } | ||
|
|
||
| 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.
|
@jinujoseph for m2 approval - a followup with a few fixes for #57140 which should go into p1 since the previous PR went into p1 |
jasonmalinowski
left a comment
There was a problem hiding this comment.
@dibarbet Nice changes, I like it!
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