Always run server handlers inside an async context

[artempyanykh]

The explanation is in the comment in the code.

In my LSP server the problem manifested itself as a server hanging during initialisation. Turned out some of the code outside of an async context raised an exception which didn't kill the process but turned the server into a potato. At the same time the client kept waiting for a response but never got one.

[baronfel]

turns the server into a potato

Amazing 🤩

[baronfel]

This is a great catch, and I think may be the root cause behind some uncaught exceptions that have been tearing down FSAC.

[razzmatazz]

hmm, there is one particular reason why I ran the handler outside the async block and that is because this is how I can ensure that LSP handler method gets called in the proper sequence, as suggested by StreamJsonRpc documentation here:

• https://github.com/microsoft/vs-streamjsonrpc/blob/main/doc/resiliency.md#nonconcurrentsynchronizationcontext

note the text saying:
"When a server method is invoked, it may already be concurrent with a previously invoked server method that has already hit its first yielding await."

I realise, I might have missed something but I believe this PR breaks serialization requirement as now run param is invoked inside the async block which runs at random order related to the other requests that got pushed to async block/thread pool

please correct me if I am wrong here

[razzmatazz]

what i am doing with this in csharp-ls is that I am registering the workload into internal queue before returning an async block in the handler when request semantics require sequencing (like textDocumentDidChange) and ensure that is run in proper sequence -- otherwise we break sequencing "commandments" from LSP, like "get definitions request should consider the results of previous did change request in-order", like detailed here:

• https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#messageOrdering

obviously this part of the LSP spec increases the complexity of the LSP server (in favor of the ability to run some parts of the handler in parallel to each other and thus increase performance), but I don't see an easy way around that..

[razzmatazz]

my suggestion would be to revert this change and rather do this (pseudo code):

let mutable asyncLspResult
try
   asyncLspResult <- run param
with Exception error:
   let rpcException = LocalRpcException(ex.Message)
   rpcException.ErrorCode <- xxxx // ???
   raise rpcException

(...)

needs to be tested though..

[artempyanykh]

Thanks for the extra context @razzmatazz! This indeed broke the ordering, so, yes, it's better to revert it and take another good look. I wonder if ordering could be fixed properly by tweaking the synchronisation context of JSON RPC though...

[razzmatazz]

Oh definitely, if you have better insights/solutions than me -- then lets do that.

The best approach would be to provide a higher-level abstraction over LSP in Ionide.LSP with automatic support for synchronization/queuing that would be done automatically for the user of the library by taking into account LSP semantics but it is over my head for now :)

[artempyanykh]

I played a bit more with the original version (= revert of this commit). A tactically inserted Async.Sleep(100) was actually enough to mess up text sync. So, I'm curious @razzmatazz @baronfel do you actually do the fully concurrent request/notification processing in your servers OR your actually properly sequence message processing yourself effectively doing a single threader request/response?

[razzmatazz]

last time I checked fsac does not support incremental text sync, always asks for a "full" document sync from the client thus even though it does not break the document content it may have sequencing problems when serializing request order.. but i think this problem was there present in fsac before conversion to streamjsonrpc in ionide.lsp; csharp-ls I have 'incremental' sync enabled thus I really need to have request ordering working right as the user may be typing fast :)

as for what i do in csharp-ls is this:

• https://github.com/razzmatazz/csharp-language-server/blob/master/src/CSharpLanguageServer/Server.fs#L1448-L1475

in particular i have this withreadwritescope decorator fn which sends the request to stateActor which actually responds (in sync) once there is no other racing request (i.e. it serializes things for me according to request semantics) -- and once i have this "confirmation" from stateActor which serializes my requests -- then only then I do return an async block from the actual handler which will run on thread pool:

• https://github.com/razzmatazz/csharp-language-server/blob/master/src/CSharpLanguageServer/Server.fs#L1411-L1426

[razzmatazz]

from my point of view LSP semantics for serialization could be pushed up into Ionide.LSP but not sure what is the right abstraction model

[artempyanykh]

Yeah, that's a tricky one. My current feeling is that doing proper concurrent/parallel processing is really hard in LSP, because although you can reorder certain things, a lot more things need to be processed strictly in-order.

I feel that serializing message processing in Ionide.LSP can probably simplify things quite a bit. The server, of course, can still do whatever background processing it should to try to service requests as fast as it can (maybe using somewhat stale data). But the inverse will just push all the synchronization complexity to the actual server implementers.

I guess another benefit of fully synced message processing is that it'd be easy to add basic support for request cancellation at the library level: the server gets $/cancelRequest, if the request is still in the queue we kill it and avoid doing the unnecessary work.

@razzmatazz do you see any potential downsides for your use-case from switching to fully sequential message processing?

[razzmatazz]

there is already support for cancellation in Ionide.LSP, see:

• https://github.com/ionide/LanguageServerProtocol/blob/main/src/LanguageServerProtocol.fs#L181
• https://github.com/microsoft/vs-streamjsonrpc/blob/main/doc/recvrequest.md#receiving-a-json-rpc-request (notes about CancellationToken)

everything plugs in rather nicely with F#'s ambient cancellation mechanism; this was one of the main points of migrating to StreamJsonRpc

i can confirm that cancellation works with charp-ls and most probably does so automatically in fsac too

[razzmatazz]

as for forcing serialization on ionide.lsp (library) level in a primitive way, I would be against it

when testing with csharp-ls, the performance and experience drops a lot because of not being able to process textdocumentdidchange while editor is waiting for response to e.g. previous "code lens" request that has not finished yet

[razzmatazz]

But the inverse will just push all the synchronization complexity to the actual server implementers.

To releave of this complexity I would rather prefer to introduce some kind of mechanism/model in ionide.lsp that does it for server implementers that takes into accoujnt LSP semantics and provides hooks to handler methods to signal when the next request can proceed or if the handler wants to block server state until the request finishes and subsequent requests should wait.

[artempyanykh]

there is already support for cancellation in Ionide.LSP

Do you mean that it's "straightforward" to add support or that it's already somehow hooked in via cancellation token? I can't seem to find anything around handling of "$/cancelRequest".

as for forcing serialization on ionide.lsp (library) level in a primitive way, I would be against it

@razzmatazz ok, that's fair. I'll do some more experimentation in my server adding the necessary synchronization points. Perhaps something more general will come out of this that we could discuss further.

[razzmatazz]

Do you mean that it's "straightforward" to add support or that it's already somehow hooked in via cancellation token? I can't seem to find anything around handling of "$/cancelRequest".

mhm.. it should work very easily, i.e. if you interact with c# libraries that accept cancellationtoken then do

let! ct = Async.CancellationToken
(.. use ct in your handler code ...)

there is an example here:

• https://github.com/razzmatazz/csharp-language-server/blob/master/src/CSharpLanguageServer/Server.fs#L196

...where I acquire ambient cancellationtoken and pass it to roslyn

f# does automatic things with ambient cancellationtoken that I myself don't really understand but it is nice that I don't have to pass it around anymore :)

[baronfel]

The StreamJsonRpc library looks at the shape of the message handler you've configured, and if a CancellationToken parameter is part of that shape it will create a CancellationTokenSource that's linked to the overall server's lifetime and pass the token from the source into your handler. @razzmatazz has hooked this up for us in requestHandling so that if you use an async as your return type the ambient token is flowed through. It's really nice.

Regarding the synchronization, I'm conflicted.

• It would be nice for ease of use of server implementers if there was a flag you could set in this library that controlled sequential processing.
• sequential processing is not required by the LSP spec, and we should allow for that as well here
• in either case, serialization errors should not crash the process.

So I think what we should do is:

• revert this commit
• try/with the serialization to prevent crashing the server
• log an issue to investigate a way for consumers to specify their desired processing mode: Sequential or Unordered

What do you two think?

[razzmatazz]

thats sounds exactly the way I think we should go

[artempyanykh]

👍
I put together #25 that does the revert followed by try-with for non-async exceptions.