Fix race causing workspace diagnostics to be stale #73653
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
| // Spin waiting until our LSP change flag has been set. When the flag is set (meaning LSP has changed), | ||
| // we reset the flag to false and exit out of the loop allowing the request to close. | ||
| // The client will automatically trigger a new request as soon as we close it, bringing us up to date on diagnostics. | ||
| while (Interlocked.CompareExchange(ref _lspChanged, value: 0, comparand: 1) == 0) | ||
| while (!HasChanged()) |
There was a problem hiding this comment.
The race was here - when there are multiple diagnostic categories (and therefore multiple requests from the client for ws diagnostics), we would only succeed is closing 1 of the requests, leaving the rest open (and stale).
That was because foreach request there is a call to WaitForChangesAsync- however only one of the waiting requests would observe the update to _lspChanged, as the first request immediately resets it back to 0 when it closes, leaving the rest still open.
This fixes the issue by storing the changed state per category, so the request for each category can separately observe the state change.
| { | ||
| // Get the LSP changed value of this category. If it doesn't exist we add it with a value of 'changed' since this is the first | ||
| // request for the category and we don't know if it has changed since the request started. | ||
| var changed = _categoryToLspChanged.GetOrAdd(category, true); |
There was a problem hiding this comment.
Slight behavior change here - since we don't know all the categories up front, we aren't able to tell on the first request if the lsp solution has changed (we don't have prior state).
So we assume it has and tell the client to re-query. This will only ever happen once on the very first request for a given category
| @@ -1926,7 +1926,12 @@ public async Task TestWorkspaceDiagnosticsWaitsForLspTextChanges(bool useVSDiagn | |||
| await using var testLspServer = await CreateTestWorkspaceWithDiagnosticsAsync( | |||
| [markup1, markup2], mutatingLspWorkspace, BackgroundAnalysisScope.FullSolution, useVSDiagnostics); | |||
|
|
|||
| // The very first request should return immediately (as we're have no prior state to tell if the sln changed). | |||
There was a problem hiding this comment.
Fallout of the behavior change above.
| /// Stores the LSP changed state on a per category basis. This ensures that requests for different categories | ||
| /// are 'walled off' from each other and only reset state for their own category. | ||
| /// </summary> | ||
| private readonly Dictionary<string, bool> _categoryToLspChanged = new(); |
There was a problem hiding this comment.
❗ If we make this a Dictionary<string, CancellationTokenSeries>, we can get rid of polling for changes altogether.
There was a problem hiding this comment.
Actually, with CancellationTokenSeries as the value, I believe we can move to ImmutableDictionary<string, CancellationTokenSeries> and also get rid of the gate.
There was a problem hiding this comment.
@sharwell I'm not seeing an easy way to use CancellationSeries here - I was initially thinking something like
private void UpdateLspChanged()
{
// Loop through our map of source -> has changed and mark them as all having changed.
foreach (var category in _categoryToLspChanged.Keys.ToImmutableArray())
{
_categoryToLspChanged[category].Token.Cancel()?
}
}
protected async Task WaitForChangesAsync(...)
{
var token = _categoryToLspChanged[category].Token?;
// Wait for token to be cancelled
...
_categoryToLspChanged[category].CreateNext();
}
but cancellation series doesn't seem to provide access to the current cancellation token. I can't have UpdateLspChanged create the next item in the cancellation series because the change event could happen before the WaitForChangesAsync method gets called (and therefore we only observe the latest, uncancelled token if though there has been a change).
There was a problem hiding this comment.
@dibarbet I implemented the described change in a local commit that builds on this change. Please let me know if you would like me to push it to this pull request.
There was a problem hiding this comment.
I implemented a version of this for review in 82dde3a
|
Will review later tonight (after 10pm) or tomorrow morning. |
| return; | ||
|
|
||
| bool HasChanged() |
There was a problem hiding this comment.
nit: i'd prefer this as a normal method, with 'category' passed in. but up do you.
|
there are probably some nicer patterns for this. However, this fixes the problem immediately in a way that fits the current pattern. Feel free to work with sam on refactorings here to other patterns if time permits. But we can get this PR in asap to address the race we have right now. |
sharwell
dismissed
their stale review
Dismissing because the polling was already present before this change
Fixes an issue I believe causing the workspace integration tests in VSCode to be flaky.
Also noticed this when running workspace diagnostics locally in VS - sometimes workspace diagnostics requests would not be closed after a didChange happened, leading to potentially stale diagnostics.