[LSP] Fix bug where request contexts were not created serially #64406
Conversation
dibarbet
added
Area-IDE
LSP
| @@ -671,7 +671,7 @@ public async ValueTask DisposeAsync() | |||
| // Some tests manually call shutdown, so avoid calling shutdown twice if already called. | |||
| if (!LanguageServer.HasShutdownStarted) | |||
| { | |||
| await LanguageServer.GetTestAccessor().ShutdownServerAsync(); | |||
| await LanguageServer.GetTestAccessor().ShutdownServerAsync("Disposing of test lsp server"); | |||
There was a problem hiding this comment.
just found this helpful when debugging tests to see who was calling shutdown.
| @@ -188,11 +188,14 @@ private async Task ProcessQueueAsync() | |||
|
|
|||
| var cancellationTokenSource = CancellationTokenSource.CreateLinkedTokenSource(CancellationToken, cancellationToken); | |||
|
|
|||
| // The request context must be created serially inside the queue to so that requests always run | |||
| // on the correct snapshot as of the last request. | |||
| var context = await work.CreateRequestContextAsync(cancellationToken).ConfigureAwait(false); | |||
There was a problem hiding this comment.
fix is here - the context must be created serially in the queue instead in StartRequestAsync which runs in parallel (non-mutating requests)
ghost
added this to the 
| @@ -201,7 +204,7 @@ private async Task ProcessQueueAsync() | |||
| // via NFW, though these errors don't put us into a bad state as far as the rest of the queue goes. | |||
| // Furthermore we use Task.Run here to protect ourselves against synchronous execution of work | |||
| // blocking the request queue for longer periods of time (it enforces parallelizabilty). | |||
| _ = Task.Run(() => work.StartRequestAsync(cancellationToken), cancellationToken); | |||
| _ = Task.Run(() => work.StartRequestAsync(context, cancellationToken), cancellationToken); | |||
There was a problem hiding this comment.
Out of curiosity I was comparing this code to the previous code, and reading the comment, and it seems like we've lost the NFW reporting it talks about:
https://github.com/dotnet/roslyn/blob/release/dev17.4/src/Features/LanguageServer/Protocol/Handler/RequestExecutionQueue.cs#L220
Is that handled elsewhere now? It's a little hard to know by just looking at the file differences, since all of the telemetry and activity ID stuff has also moved.
There was a problem hiding this comment.
indeed we did. However this is in the common layer so I'm not exactly certain how to fix this
There was a problem hiding this comment.
ahhh yes, of course. layering
When we switched over to clasp we inadvertently introduced a bug where request contexts started being created in parallel for non-mutating requests. This was caught by flaky tests, e.g. https://dev.azure.com/dnceng-public/public/_build/results?buildId=35727&view=ms.vss-test-web.build-test-results-tab&runId=712518&resultId=211420&paneView=dotnet-dnceng.dnceng-build-release-tasks.helix-test-information-tab