Dev/toddgrun/request concurrency #66767
Conversation
…utionQueue. This replaces the MutatesServerState flag with a flag that indicates how the request should be treated. Specifically, I needed a mode where the request ensured that all prior pending requests had completed before processing.
…UponMutatingRequest
Remove an IsCancellationRequested check. Throw some InvalidOperationExceptions based on unreachable conditions.
…, using NoThrowAwaitableInternal, and slightly different exception handling
…b.com:ToddGrun/roslyn into dev/toddgrun/RequestConcurrency
|
I'd vote we just move forward with this one, the other one is a bit of a mess. |
|
CC @dibarbet, @CyrusNajmabadi, @jasonmalinowski, and @davidwengier. Since they reviewed the parent of this PR. |
|
My previous strategy for the |
|
@dibarbet @CyrusNajmabadi @jasonmalinowski and @davidwengier I've abandoned the old PR in favor of this one, can y'all take a look at this one sometime early this week? If I'm going to have any chance of getting the work done on my side in preview 2, I'll need this available fairly soon. |
| { | ||
| // Explicitly ignore this exception as this can occur during the CreateLinkTokenSource call, and means one of the | ||
| // linked cancellationTokens has been cancelled. | ||
| } |
There was a problem hiding this comment.
i feel like this needs to move to the place where we make the linked token. i think we should handle this clearly and specify the exact semantics of what that means at that location.
There was a problem hiding this comment.
I debated on this when I made the change. The two alternates that I saw utilizing a tighter catch around the CreateLinkedTokenSource call were to catch and then either throw an OperationCanceledException or to catch and then do a continue to skip the rest of that iteration. I'll try out the latter and see if you like it better.
src/Features/LanguageServer/Microsoft.CommonLanguageServerProtocol.Framework/TaskExtensions.cs
Show resolved
Hide resolved
| } | ||
|
|
||
| concurrentlyExecutingTaskCts.Dispose(); | ||
| }, CancellationToken.None, TaskContinuationOptions.ExecuteSynchronously, TaskScheduler.Default); |
There was a problem hiding this comment.
nit: doc that CT.None here is very intentional as this code is absolutely necessary to run for clearnup regardless of how the task completed, and regardless if things were cancelled.
| @@ -0,0 +1,138 @@ | |||
| // Licensed to the .NET Foundation under one or more agreements. | |||
There was a problem hiding this comment.
not reviewing. trusting it was copied minimally.
ghost
added this to the 
This is a descendent of #66727 to try to take some work on the CLaSP core off of Todd's plate. @ToddGrun is welcome to cherry-pick my commits off here, rework them or we came merge this one instead, whatever works in the end.
Mostly what I've done is just to add a unit test as requested by DavidB.