CLaSP: Enable scenario for Drain and cancel the queue when there's a mutating request

[ryanbrandenburg]

The WebTools folks would like to be able to have the RequestExecutionQueue Cancel all pending requests and wait for already started tasks to finish (in case things didn't check for cancellation) when a mutating request comes in. This is a requirement for them because they don't have Snapshots of their tree (It's not immutable).

We think we can enable this scenario by wrapping these lines in a virtual method and then allowing them to override it, but depending on how complicated it is to implement we might include it as a (non-default) option for CLaSP.

[davidwengier]

Do you mean clear the queue? Draining the queue implies to me that it will spin through and dequeue everything, so if things "don't check cancellation tokens" that's just going to cause a bunch of work. Clearing the queue and just forgetting about anything pending makes sense though, and doesn't really have anything to do with cancellation tokens, since a task has to be dequeued and run for it to even look at the token.

[ryanbrandenburg]

"Drain the queue" was the wrong wording here (will edit). They need items which have already begun executing to no longer be running (one way or another) before the mutating request can continue. So it's not enough for the queue to just "forget" about already running tasks (which we already do), and we've no reason to bother items which have not yet been de-queued.

Better phrasing would have maybe been "finish remaining tasks".

[davidwengier]

That makes more sense, and I guess a "wait for queue to empty before starting mutating request" would be a reasonable feature. It could cancel, but would have to wait anyway, so doesn't matter if the tasks themselves support cancellation.

Though I do wonder if there is much realistic benefit to this, over just having them mark all of their handlers as mutating. Seems like if they're not immutable then parallel processing might be a bad idea regardless.

[ryanbrandenburg]

"Wait for queue to empty" is not the issue (my bad wording to start no doubt confused things here), the items behind the mutating request in the queue are unaffected by the changes WebTools need to make. Let's imagine a scenario like this:

Queue:
[0]: textDocument/didChange mutating
[1]: textDocument/semanticTokens/range nonmutating
[2]: textDocument/didChange mutating

In Flight (already off the queue, currently executing in their handler)
[0]: textDocument/semanticTokens/range nonmutating
[1]: textDocument/outlining nonmutating
[2]: textDocument/highlighting nonmutating

Now the Queue de-queue's queue item 0, which is mutating, so before it starts executing it needs to cancel all In-Flight items then wait for them to finish (one way or another) before proceeding. Otherwise the tree the "In Flight" requests are operating on may mutate in the middle of the request.

Marking all their handlers as mutating would in effect make the entire server single-threaded, which I assume (admittedly without data) would have significant performance impacts in scenarios where you're just "exploring" the code. Opening a file for the first time in a session comes to mind as a particularly dangerous perf area there.

CC @ToddGrun and @jimmylewis in case I'm misrepresenting/mis-understanding their needs.

[jimmylewis]

To put it in the terms Parallel vs Serial like OmniSharp uses, we need to ensure that our Serial requests are not executing in parallel with any outstanding Parallel requests.
To put it in terms of read vs. write, we need to ensure that writes are not occurring while any reads are in progress.

Since most requests are parallel/read-only/non-mutating, we want to keep as much parallel execution as possible. On a given keystroke/didChange notification, we usually see several follow up requests. We need better safety when a mutating request comes in. Ryan's example spells this out nicely.

[ToddGrun]

I’ve made some changes with regards to seeing what adding the equivalent of O#’s WithContentModified(false) to the RequestExecutionQueue and I ended up with the changes here. Would love for your thoughts on whether this approach would work. (I’ve done some local verification and things seem to hold together and I know that these are breaking changes that will require corresponding razor changes)

[ryanbrandenburg]

Fixed by dotnet/roslyn#66767.