Use stream json rpc for transport #10

razzmatazz · 2021-12-28T17:53:07Z

This enables us to avoid mucking with low-level stuff and get support for $/cancelRequest automatically
Still needs work, esp type Client is not reimplemented
Please ignore the remove paket commit, that is an optional part
works with CSharpLanguageServer where the main wish was to enable cancellation support which this PR does

razzmatazz · 2021-12-28T17:54:50Z

@baronfel this is a continuation (alternative implementation) of #6 where I converted Server to use StreamJsonRpc lib like you suggested.

this is somewhat functional (apart from reporting errors via LspResult<result>`) and probably misc stuff

does this look ok to you? (please check only the second commit, 0f968fe)

razzmatazz · 2022-01-17T21:28:13Z

src/LanguageServerProtocol.fs

-            | :? JsonException as ex ->
-                async.Return (Result.Error (Error.Create(ErrorCodes.parseError, ex.ToString())))
+            let runAsTask param ct =
+                Async.StartAsTask(runAndUnwrap param, cancellationToken=ct)


@baronfel I am in a bit of a pickle here with request ordering

Basically, StreamJsonRpc provides good request ordering, but it does that in a TPL/C# Task-specific way.. it ensures another task delegate is not invoked until the current one returns a Task so the application can have a change to enqueue current request to internal queue/some kind of lock object before allowing the next one to come in. See first paragraph in https://github.com/microsoft/vs-streamjsonrpc/blob/main/doc/resiliency.md#nonconcurrentsynchronizationcontext

If I were to implement it in C# then it would be relatively ok, as I could just do all the queuing of the work in non-async code before returning a Task from the delegate method. But in F# I am forced to launch Async.StartAsTask, if I want to attach a cancellation token to async workflow... And F# async are "cold" tasks.. So I was mucking with this code a bit and I probably don't know how to accomplish that thing with current prototype for (run: 'server -> 'param -> AsyncLspResult<'result>).

Maybe you have an idea. Otherwise I will have to change the prototype of run to return a Task<LspResult> instead to account for StreamJsonRpc sequencing signaling in my handler code that does enqueue text changes ("patches") from the client to roslyn model in good sequence before returning a Task with remainder of the handler code that can then be run on task pool as it is ok at that point for me.

Is there some way to set Async.CancellationToken w/o invoking Async.StartAsTask..? I am not sure myself what would be the correct approach before converting the thing to use tasks..

This is mostly evident/uncovered in csharp-ls when there are multiple textDocument/didChange requests from the client and those are dumped to TPL task pool as they come in by Async.StartAsTask and are executed randomly(?) by the thread pool code. The result is that csharp-ls processes document deltas in wrong order and the document model gets corrupted. I was hoping I could get by with Async.StartAsTask hack but alas..

It's probably ok to just accept needing to use tasks. That's what I did with my prior attempt, and we have good interop with that now in f# 6/with ply et al.

Hmm, then I will have to pass in ct to every request handler, that patch will get ugly :/

Yeah I know. It was a lot of churn in FSAC when I attempted it in the past, but I think it's worth it to take the pain. We get a well supported and fast json rpc base, and in exchange we have to be a bit more explicit about things.

hmm, I actually managed to avoid the need to convert everything to task-based flow. I simply do it like for tasks -- return async computation/object/block (not sure what is the right name for that ?) at the last moment and thus I have a chance to obtain state locks and to not drop to TPL thread pool until I have things in place when handling a request.

I will need to update this PR later.

Also, to make it easier for me to compose handler behaviour I will introduce another version of start function that does not require a server "class" -- just a map of handlers. I loose some of type checking (I guess..) but I can compose the behaviour more easily with functional techniques. I will leave the original version in place too, so we should be good.

* Rename requestHandling -> serverRequestHandling; * Add requestHandling that does not depend on LspServer instance (for more functional implementations); * Add startWithSetup so we can code a server w/o LspServer instance.

razzmatazz · 2022-03-05T17:49:46Z

@baronfel this is up for review.. some very questionable :) changes were made, it is very up for discussion:

this has been tested with csharp-ls for a couple of months by me (and other ppl presumably, nothing bad reported AFAIK)
I have added API so I can build a server w/o a LspServer instance -- that allows me to do some more decoration on handler functions when setting up the server (not so easy for member methods), see:
- https://github.com/razzmatazz/csharp-language-server/blob/master/src/CSharpLanguageServer/Server.fs#L1077-L1098
I did not touch module Client (did not convert it to StreamJsonRpc) -- not sure how to test it, so it still uses "old" internal JsonRpc module -- which should eventually be removed

as i said in this comment, there was actually no problem serializing Task vs Async processing as in both cases I can just run sync code with locks until the very point I actually launch the handler which is then run asynchronously no problem

razzmatazz · 2022-03-05T17:54:41Z

sorry it took so long, got my new pc delivered, then it broke for weeks, then I was tracking emacs bug related to dotnet (https://lists.gnu.org/archive/html/emacs-devel/2022-02/msg00009.html) -- then life happened :)

razzmatazz · 2022-03-06T08:40:36Z

also it is somewhat a shame paket does not suport paket.local for .net core projects :( -- really hard to test things when you have to start your own nuget server to test your project integration before publishing

complaining because csproj/sln does not this problem (even if it has other issues). this is why i was pushing #8

razzmatazz · 2022-03-19T18:41:08Z

sorry for bugging @baronfel but does this need more work from me before getting it merged?

razzmatazz · 2022-04-03T16:36:02Z

ping @baronfel

baronfel · 2022-04-05T03:21:18Z

Hey! Sorry this took so long for me to get back to - it's great! Really excited to have this in :)

razzmatazz marked this pull request as draft December 28, 2021 17:53

razzmatazz mentioned this pull request Jan 3, 2022

Use StreamJsonRpc to implement LanguageServerProtocol.Server ionide/FsAutoComplete#877

Closed

razzmatazz mentioned this pull request Jan 12, 2022

Doesn't work with latest Emacs git master? razzmatazz/csharp-language-server#12

Closed

razzmatazz commented Jan 17, 2022

View reviewed changes

razzmatazz force-pushed the stream-json-rpc branch from 0c07bb9 to 7efcfa4 Compare January 24, 2022 21:08

razzmatazz added 3 commits March 5, 2022 14:00

Add ClientInfo to InitializeParams

c6a789b

Add StreamJsonRpc dependency

0528f23

Use StreamJsonRpc to implement Server

c4924f4

* Rename requestHandling -> serverRequestHandling; * Add requestHandling that does not depend on LspServer instance (for more functional implementations); * Add startWithSetup so we can code a server w/o LspServer instance.

razzmatazz force-pushed the stream-json-rpc branch from 7efcfa4 to c4924f4 Compare March 5, 2022 17:44

razzmatazz marked this pull request as ready for review March 5, 2022 17:44

Use 6.0.200 SDK in global.json

2edb81f

razzmatazz mentioned this pull request Mar 19, 2022

rough support for $/cancelRequest #6

Closed

baronfel changed the title ~~Use stream json rpc~~ Use stream json rpc for transport Apr 5, 2022

baronfel merged commit 425ccf8 into ionide:main Apr 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use stream json rpc for transport #10

Use stream json rpc for transport #10

razzmatazz commented Dec 28, 2021

razzmatazz commented Dec 28, 2021 •

edited

razzmatazz Jan 17, 2022 •

edited

baronfel Jan 17, 2022

razzmatazz Jan 17, 2022

baronfel Jan 17, 2022

razzmatazz Feb 4, 2022 •

edited

razzmatazz commented Mar 5, 2022 •

edited

razzmatazz commented Mar 5, 2022

razzmatazz commented Mar 6, 2022

razzmatazz commented Mar 19, 2022

razzmatazz commented Apr 3, 2022

baronfel commented Apr 5, 2022

Use stream json rpc for transport #10

Use stream json rpc for transport #10

Conversation

razzmatazz commented Dec 28, 2021

razzmatazz commented Dec 28, 2021 • edited

razzmatazz Jan 17, 2022 • edited

Choose a reason for hiding this comment

baronfel Jan 17, 2022

Choose a reason for hiding this comment

razzmatazz Jan 17, 2022

Choose a reason for hiding this comment

baronfel Jan 17, 2022

Choose a reason for hiding this comment

razzmatazz Feb 4, 2022 • edited

Choose a reason for hiding this comment

razzmatazz commented Mar 5, 2022 • edited

razzmatazz commented Mar 5, 2022

razzmatazz commented Mar 6, 2022

razzmatazz commented Mar 19, 2022

razzmatazz commented Apr 3, 2022

baronfel commented Apr 5, 2022

razzmatazz commented Dec 28, 2021 •

edited

razzmatazz Jan 17, 2022 •

edited

razzmatazz Feb 4, 2022 •

edited

razzmatazz commented Mar 5, 2022 •

edited