Add support for two-way communication #127

yyoncho · 2022-02-15T09:22:44Z

implemented streamconnection.nim which aims to provide symetric
communication between two parties. In addition to the two-way communication I
have intruduced notifications (via StreamConnection.notify).
Added faststreams to allow using asyncdispatch/chronos. This may cause
breakages in the nim-json-rpc clients which will at least need to specify
asyncBackend=chronos (and eventually add import chronos because I have removed
the export from router.nim/server.nim)
Documentation is not part of the PR and will be added before the PR is merged.
changed router.nim to allow passing the full jsonrpc request(not only the
params) to allow implementing cancel requests by having a way to pass the id
to the handler.
Fixed several import not used and deprecated warnings

The main goal of this PR is to make `nim-json-rpc` appropriate for `Language Server Protocol` server implementation. - Implemented support for passing the message length via `Content-Length` (typically over stdio/TCP). - implemented `streamconnection.nim` which aims to provide symetric communication between two parties. In addition to the two-way communication I have intruduced notifications (via StreamConnection.notify). - Utilized `faststreams` to allow using `asyncdispatch`/`chronos` conditionally. This may cause breakages in the `nim-json-rpc` clients which will at least need to specify `asyncBackend=chronos` (and eventually add import `chronos` because I have removed the export from `router.nim`/`server.nim`) - Documentation is not part of the PR and will be added before the PR is merged. - changed router.nim to allow passing the full `jsonrpc` request(not only the `params`) to allow implementing `cancel` requests by having a way to pass the `id` to the handler. - Fixed several import not used and deprecated warnings

arnetheduck · 2022-02-15T09:42:14Z

json_rpc/streamconnection.nim

+  write(OutputStream(connection.output), value)
+  flush(connection.output)
+
+proc readMessage*(input: AsyncInputStream): Future[Option[string]] {.async.} =


async input streams based on faststreams require that all consumers of the new API are based on faststreams/async - this significantly limits the utility of the feature - what should be done instead is to move the main logic such that it can be used with "pure" chronos without faststreams, then the faststreams support should be implemented as an optional layer on top that can be imported / enabled separately.

I just want to point out that start is generic, and you can implement readMessage for whatever entity you want. I had an implementation over the sync stream(which I dropped before making this PR). Not sure if this will address your concern at 100%.

Unfortunately not - the requirement that a specific -d flag is used is the problem / non-starter: it "infects" unrelated usage of faststreams globally - thus, it's better to either stick with pure chronos, or develop the faststreams support on the side - this latter option needs work on the faststreams side as well.

arnetheduck · 2022-02-15T09:44:35Z

json_rpc/client.nim

 method call*(client: RpcClient, name: string,
             params: JsonNode): Future[Response] {.
-    base, async, gcsafe, raises: [Defect, CatchableError].} =
+    base, async, gcsafe, raises: [Defect, CatchableError, Exception].} =


throughout, Exception should never appear as part of the raises list - if it "leaks" from somewhere, it should be contained in that place by correctly annotating that source, or worst case, surrounding the code with try: ... except CatchableError as exc: raise exc except Exception ass exc: raiseAssert exc.msg - usually Execption infections come from incorrect forward and/or proc declarations.

I will track down that. I believe it was caused by async when used in combination with asyncdispatch but I have to verify that.

we generally don't support / work with asyncdispatch (it doesn't work in faststreams for example), so that would be surprising - importantly though, raises lists are infectious.

ah, I also see that this function in particular is async - those should not have raises lists at all (chronos does not yet fully support raises lists for async functions and adding them causes trouble - the correct annotation for an async function is raises: [Defect] which should be done with a push raises: [Defect] on top of every file

FYI, here is the root cause:

import std/asyncdispatch proc echoString(params: string): Future[string] {.async, raises: [Defect, CatchableError].} = return params

Fails with

/home/yyoncho/Sources/nim/Nim/lib/pure/asyncmacro.nim(232, 33) Error: echoStringNimAsyncContinue_436207620() can raise an unlisted exception: Exception

yes, so what async does is turn the function into two functions - one "wrapper" that creates a future and one closure that contains the body that you wrote - the closure catches all exceptions and puts them in the future, generally - the raises annotation is applied to the "wrapper", not the "user code" (as it perhaps should be, but this has other problems), hence the correct raises annotation for async is raises: [Defect], with async functions - if asyncdispatch fails here, it's a deeper issue with AD that needs to be resolved in the std lib most likely - we require that no exceptions are raised from the "wrapper", and enforce that in most projects (nim-json-rpc is just a little bit behind in maintenance - more info:

https://status-im.github.io/nim-style-guide/errors.html

https://github.com/status-im/nim-chronos#exception-effects

arnetheduck · 2022-02-15T09:52:46Z

This may cause breakages in the nim-json-rpc clients which will at least need to specify asyncBackend=chronos

nim-json-rpc needs to continue to work for non-async-faststreams consumers - the best way to achieve that is by putting core logic in one place (usually together with "pure" chronos support), then adding faststreams support as a wrapper through a separate module - ideally, faststreams itself should be split into multiple modules such that the async support is separated out - it is experimental and not necessary the route forwards for all projects that use sync faststreams.

The -d approach was done as a quick and dirty hack to disable the experimental behaviors, but it's not a good long-term solution as it lacks composability: projects cannot specify per-library options this way.

yyoncho · 2022-02-15T10:19:09Z

the best way to achieve that is by putting core logic in one place (usually together with "pure" chronos support)

Can you elaborate on that? My understanding is:

I need asyncdispatch for the communication with pipes
I cannot have chronos used and exported by router.nim/server.nim because it will collide with asyncdispath
To solve 1/2 I am using faststreams/async_adapter so I can use conditionally async from chronos/asyncdispath based on the asyncBackend

I can solve 1/2 using some conditional imports but that won't be very different from using faststreams. Just for the record, if I solve 1/2 somehow I don't need faststreams - I can read directly from the pipe.

arnetheduck · 2022-02-15T10:28:48Z

chronos supports pipes, like so: https://github.com/status-im/nim-chronos/blob/master/tests/teststream.nim#L1236

the main missing feature I know of of is async process I/O - implementing this would require porting parts of https://github.com/cheatfate/asynctools to chronos - this should be straightforward, and has been on our TODO list for some time.

yyoncho · 2022-02-15T11:03:28Z

the main missing feature I know of of is async process I/O - implementing this would require porting parts of https://github.com/cheatfate/asynctools to chronos - this should be straightforward, and has been on our TODO list for some time.

Yes, that is the limitation I was referring to(sorry for the vague comment).

I see a few options here:

Implement async process I/O in chronos and go with chronos (no need from faststreams, etc).
Implement somehow support simultaneously for chronos/asyncdispatch in nim-json-rpc in a way that won't affect current clients. I am not sure how this can be done(I believe I can use include and have core logic in separate file and then wrappers for chronos/asyncdispath).
Switch to using TCP for the lsp server. This way, I will be able to implement streamconnection over it(and even merge it with socketserver.nim)

From my POV 3) is the most deterministic option.

arnetheduck · 2022-02-15T12:02:20Z

either of 1) and 3) are fine in my book - 3 is easier to get done quickly, 1 is perhaps the better long-term strategy but somewhat messy due to cross-platform support

yyoncho · 2022-02-15T13:31:51Z

Ok, I will implement 3. If we at some point have 1) then it will be easy to add pipes support on top of 3)

zah · 2022-02-15T13:43:46Z

@arnetheduck,

The Nim language server is intended for upstream adoption by the Nim team. The use of Chronos there instead of the standard library will be controversial. Furthermore, Chronos lacks support for async process I/O, which will limit the capabilities of the language server (it won't be able to collect output of program executions such as nim check).

These considetations were the reason why the code is based on FastStreams streams and its integration with AsyncTools, which are compontents we are already using in the Chronicles Tail tool.

zah · 2022-02-15T13:44:50Z

With the right defaults, clients of nim-json-rpc won't be affected in any way by the changes introduced here.

arnetheduck · 2022-02-15T19:59:16Z

The Nim language server is intended for upstream adoption by the Nim team.

In that case, we need to start over from 0: no stew, no nim-serialization, no faststreams, no nim-json-ser, no nim-json-rpc and so on (given that all these reimplement std modules one way or another) - chronos is one of the less controversial choices - there have been discussions to deprecate asyncdispatch / move it out of std for nim 2.0.

Long story short, it seems more viable to provide upstream with nimsuggest improvements - specially given the odd design with the LS being mostly a process manager - it belongs in a separate project no matter what: there's literally no benefit of tying its development lifecycle to that of Nim (with its yearly releases), specially not at this early stage. Once it works, we can have a discussion with upstream what to do with it.

Furthermore, Chronos lacks support for async process I/O

this is easily portable, should it be needed - it's not like it's black magic to write it for chronos, but rather that we haven't really had a reason to - further, asynctools (where the process support for asyncdispatch comes from) has been abandoned - it doesn't bode well for its usage.

arnetheduck · 2022-02-15T20:02:51Z

AsyncTools, which are compontents we are already using in the Chronicles Tail tool.

this is interesting, because last I checked (when trying to add tests), faststreams didn't even compile with asyncdispatch ;) that said, I don't think anyone compiles chronicles tail either, so that could be a reason nobody has noticed.

zah · 2022-02-15T21:26:14Z

In that case, we need to start over from 0: no stew, no nim-serialization, no faststreams, no nim-json-ser, no nim-json-rpc and so on (given that all these reimplement std modules one way or another) - chronos is one of the less controversial choices - there have been discussions to deprecate asyncdispatch / move it out of std for nim 2.0.

We are now in the process of making the first release of the Nim langauge server which already works better than any of the alternatives. Why should we delay that exactly? Furthermore, none of the libraries you cited is incompatible with the standard library in the same way Chronos is.

this is easily portable, should it be needed - it's not like it's black magic to write it for chronos, but rather that we haven't really had a reason to - further, asynctools (where the process support for asyncdispatch comes from) has been abandoned - it doesn't bode well for its usage.

This doesn't seem to be the case since I've requested it many times and it still hasn't been delivered few years later.

this is interesting, because last I checked (when trying to add tests), faststreams didn't even compile with asyncdispatch ;) that said, I don't think anyone compiles chronicles tail either, so that could be a reason nobody has noticed.

Well, your testing protocol was wrong because both Chronicles Tail and more importantly the new language server compile and work fine with these components being used.

arnetheduck · 2022-02-16T07:05:00Z

We are now in the process of making the first release of the Nim langauge server which already works better than any of the alternatives. Why should we delay that exactly?

I didn't write one should delay releasing it - what I'm saying is that putting it in the main nim repo is far off, if ever - the dependencies is the small reason, the big reason is that there's no need to and no point in doing so.

arnetheduck · 2022-02-16T07:05:48Z

Well, your testing protocol was wrong

see the disabled test in nim-faststreams' nimble file - I just tried to run the test suite with ad

arnetheduck · 2022-02-16T07:07:30Z

Furthermore, none of the libraries you cited is incompatible with the standard library in the same way Chronos is.

no, but they are also not slated for std inclusion - the nim project doesn't take any dependencies as acceptable, specially not if they duplicate std modules, like the cited modules do

arnetheduck · 2022-02-16T07:48:55Z

actually, the other thing we could/should do is to add support to choosenim such that it can install it alongside nim - this should be feasible, and gives us the freedom to use whatever libraries we want / maintain / etc - rustup for example installs the rust language server

zah · 2022-02-16T11:01:47Z

The Nim language server lives in its own repo:
https://github.com/nim-lang/langserver

It has an independent release schedule and it's not tied to a particular version of Nim (one of the main advantages of the new language server over the existing one from PMunch is that it can work with multiple versions of Nim at the same time, even within a single project). We've been granted write access to the nim-lang repo and we'll be in charge of the release schedule.

arnetheduck · 2022-02-16T11:35:02Z

Well, then all is well, and I don't see any reason not to use code we've developed to be stable - we will not be able to use asyncdispatch over multiple Nim releases regardless due to the bugs in it, which are plentiful, and the lack of maintenance - even if we fix bugs in it, we would effectively be tied to the nim release cycle transitively - same for cancellation support, ssl, and all the other reasons we have chronos for (AD does get some more maintenance than asynctools, but it's still buggy and undermaintained) - it's simply a path forwards that will be difficult, if not impossible, to maintain productively.

arnetheduck · 2022-02-16T11:36:49Z

The core issue here is that we develop a lot of stuff on top of chronos, which is the main reason it gets maintenance and attention - also developing everything for asyncdispatch is not viable over time (nim-ws, nim-json-rpc, etc etc) - the corpus will grow over time making it less and less feasible to maintain both - we don't have the bandwidth for that, really.

zah · 2022-02-16T14:32:56Z

I think we can revisit this in the future. Spending time on implementing async I/O support in Chronos seems counter-productive when we can readily ship the first version of NimLS with already available components that work well enough (admittedly, we have a rather low bar to jump over at the moment).

When we can no longer make rapid improvements in developer productivity by improving nimsuggest and the NimLS internal logic and when we start suffering from the issues you are alluding to (and that's a speculation), it would be wise to address the technical debt created here.

zah · 2022-02-16T14:39:08Z

I think there is also another potential for misunderstanding of the changes here.

This PR is not a risky conversion of the lower layers of the library to FastStreams. It merely creates another way to instantiate the library (as opposed to creating a WebSocketServer or a HttpServer) which is necessary for the particular way a language server is expected to operate over stdin/stdout. Arguably, the use of FastStreams makes the new module more versatile, not less so. What is important to understand is that all existing modules continue to function just as they did before the PR.

arnetheduck · 2022-02-16T19:06:45Z

json_rpc/router.nim

+    params[0].kind != nnkEmpty:
+    result = true
+
+proc route*(router: RpcRouter, node: JsonNode): Future[RpcResult] {.async, gcsafe.} =


The change to RpcResult is also an API-breaking change that will upset existing consumers

Is the router used outside of nim-json-rpc?

Well, it's public, so it can be used - this is a bit of a messy aspect of Nim, the module system is shifty - a first litmus test would be to check if nimbus-eth1 and nimbus-eth2 still compile after the change - if these to projects compile, it's mostly fine for a library like json-rpc where we don't yet carefully maintain backwards compat

arnetheduck · 2022-02-16T19:07:24Z

json_rpc/server.nim

@@ -1,10 +1,10 @@
 import
  std/tables,
-  chronos,
+  faststreams/async_backend,


why would fs be needed here?

to support the ability to compile in chronos/asyncdispatch mode(i. e. providing async)

but faststreams is not used in this module? perhaps you need an export somewhere else

faststream/async_backend is used only to export conditionally chronos or asyncdispatch stuff based on asyncBackend.

right - so with asyncBackend=none (the default), nothing will be exported by async_backend - this is how we use the library generally in projects.

kdeme · 2022-02-17T08:33:29Z

json_rpc.nimble.cfg

@@ -0,0 +1,2 @@
+--define:"async_backend=asyncdispatch"
+--threads:on


will be added.

kdeme · 2022-02-17T08:34:20Z

json_rpc/router.nim

@@ -1,19 +1,21 @@
 import
-  std/[macros, options, strutils, tables],
-  chronicles, chronos, json_serialization/writer,
+  std/[macros, options, strutils, tables], sugar,


sugar can go in the std brackets

arnetheduck · 2022-02-17T16:55:04Z

It merely creates another way to instantiate the library

The problem is that this is a leaky abstraction - if we require -d:asyncBackend=asyncdispatch, this will obviously fail for chronos - we also can't require -d:asyncBackend=chronos in nimbus, as this will break code generally, because faststreams will start exposing new symbols and redefine the objects to include async support - nimbus compiles with -d:asyncBackend=none currently and must continue to do so.

arnetheduck · 2022-02-17T17:00:46Z

to allow implementing cancel requests by having a way to pass the

in chronos, cancellation is done via calling Future.cancel - asyncdispatch lacks support here, but for "chronos" mode, we need to not expose the id-based cancel and instead make sure that the library works with the native cancellation

yyoncho · 2022-02-17T17:07:27Z

in chronos, cancellation is done via calling Future.cancel - asyncdispatch lacks support here, but for "chronos" mode, we need to not expose the id-based cancel and instead make sure that the library works with the native cancellation

We have discussed that with @zah, and that won't work for the cases that LSP needs because we have to either propagate the cancelation or keep the sub-futures running.

arnetheduck · 2022-02-17T17:29:22Z

to either propagate the cancelation or keep the sub-futures running.

right - this creates a bit of a problem with the faststreams approach in general: on the surface it looks like an easy way to get started, but the practical implications of trying to support both AD and chronos in the same library are far-reaching and difficult to reason about / resolve in a principled manner, given the nature of the differences in the two async libraries - it's hard to motivate adding id-based cancellation to this library and not follow the chronos standard in this area - likewise for exception / error handling support, propagation etc.

One option might be to simply keep this code in a branch / fork, until async process i/o is added to chronos - this might be the simplest / least bad idea for now - then none of the above concerns apply and we can get back to the problem later.

yyoncho · 2022-02-17T20:59:28Z

One option might be to simply keep this code in a branch / fork, until async process i/o is added to chronos - this might be the simplest / least bad idea for now - then none of the above concerns apply and we can get back to the problem later.

If we decide to go with chronos we could use tcp. In other words, the lack of async process i/o is not a blocker for using chronos.

arnetheduck reviewed Feb 15, 2022

View reviewed changes

yyoncho requested a review from zah February 15, 2022 12:12

arnetheduck reviewed Feb 16, 2022

View reviewed changes

kdeme reviewed Feb 17, 2022

View reviewed changes

		@@ -0,0 +1,2 @@
		--define:"async_backend=asyncdispatch"
		--threads:on

Add support for two-way communication #127

Are you sure you want to change the base?

Add support for two-way communication #127

Conversation

yyoncho commented Feb 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnetheduck Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

arnetheduck commented Feb 15, 2022

yyoncho commented Feb 15, 2022

arnetheduck commented Feb 15, 2022

yyoncho commented Feb 15, 2022

arnetheduck commented Feb 15, 2022

yyoncho commented Feb 15, 2022

zah commented Feb 15, 2022

zah commented Feb 15, 2022

arnetheduck commented Feb 15, 2022 • edited Loading

arnetheduck commented Feb 15, 2022

zah commented Feb 15, 2022 • edited Loading

arnetheduck commented Feb 16, 2022

arnetheduck commented Feb 16, 2022

arnetheduck commented Feb 16, 2022

arnetheduck commented Feb 16, 2022

zah commented Feb 16, 2022

arnetheduck commented Feb 16, 2022

arnetheduck commented Feb 16, 2022

zah commented Feb 16, 2022 • edited Loading

zah commented Feb 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnetheduck commented Feb 17, 2022

arnetheduck commented Feb 17, 2022

yyoncho commented Feb 17, 2022

arnetheduck commented Feb 17, 2022

yyoncho commented Feb 17, 2022

arnetheduck Feb 17, 2022 •

edited

Loading

arnetheduck commented Feb 15, 2022 •

edited

Loading

zah commented Feb 15, 2022 •

edited

Loading

zah commented Feb 16, 2022 •

edited

Loading