Re: New jrpc.el JSONRPC library

[João Távora]

Philipp Stephani <address@hidden> writes:

> João Távora <address@hidden> schrieb am Fr., 18. Mai 2018 um 18:28 Uhr:
>
>>  I propose this library, jrpc.el, for Emacs core (or GNU ELPA, or

> Thanks. I think that separating out JSONRPC from LSP and having a
> JSONRPC library in core are highly valuable additions that I'd
> strongly support.  Some review comments follow; sorry that there are
> so many, but I think as a foundational library jrpc.el should be as
> high-quality as possible:

Apology *not accepted* :-). Having reviewers provide so many insightful
comments is a previlige and a pleasure. The high standards of the Emacs
project are one of the reasons I like to contribute to it.

I'll triaged your comments into "No", "Maybe", "Ok". The former require
some more convincing on your part :-)

I've already applied some of your suggestions (and some that Stefan sent
off-list, too). See the jsonrpc-refactor branch of Eglot's github
repository. The latest version of jsonrpc.el (I changed the name) lives
in:

https://raw.githubusercontent.com/joaotavora/eglot/jsonrpc-refactor/jsonrpc.el

> - Please add extensive unit and integration tests. There are lots of
> corner cases (what happens when the server times out, sends an invalid
> or incomplete response, sends responses in the wrong order, etc.) that
> should all be tested.

OK. It's in the plans, if not in the stars. Help appreciated. :-) We can
convert some of those examples to tests.  Eglot's jsonrpc-refactor
branch, where I'm currently developing jsonrpc.el, already has
integration tests, so at least a part of jsonrpc.el is already covered.

> - Please remove all defcustoms and defvars. jrpc.el is a low-level
> library, it doesn't make sense for it to maintain global state or be
> globally customizable. Rather, the timeout could either be a mandatory
> argument, or a reasonable constant default could be selected, and the
> other variables should be per-process.

OK. Makes sense. Done.

> - Also please remove `jrpc-current-process'. There is no general
> notion of a "current" process;

Maybe. Not convinced. It's opininated, sure. It supposes that just most
JSONRPC applications would have the notion of a "session". So jsonrpc.el
makes this suggestion to its users, allowing them to configure what a
session means for them. They would also get access to M-x
jsonrpc-events-buffer. This is entirely optional on the client's part,
but I see no harm in JSONRPC providing this convenience. Perhaps the
name is off and should mention "session".

> - Please don't use the `error' symbol directly. Instead, define your
> own error symbols for each error category using `define-error'. Only
> then clients have the chance to catch and handle errors.

OK. Done. Though technically they can already catch and handle
them. What they can't do is catch *only* JSONRPC errors and let the
others thru. I happen to think that's a bad idea because a JSONRPC error
is an error in your program. But there's no reason to deny this, too.

> - `jrpc-define-process-var' isn't JSONRPC-specific, it should be moved
> somewhere else (subr.el)?

Yes, but if we do this it'll be harder for clients like eglot.el to keep
supporting Emacs 26.1 (assuming jsonrpc.el would be in GNU ELPA and
master simultaneously).

> - Instead of defining process variables, consider using a structure or
> EIEIO class to encapsulate all needed state for the process client.
>
Maybe. Structures is a bad idea, but defclass isn't at all.

> - Please make it possible to run servers remotely. Unfortunately this
> isn't possible with `make-process', so you have to use
> `start-file-process' instead if `default-directory' is remote. As an
> alternative to using `default-directory', consider passing a
> REMOTE-DIRECTORY argument to `jrpc-connect' so that the behavior
> doesn't depend on ambient context.

Maybe. I don't fully understand the example. But jsonrpc-connect allows you to
pass any pre-connected process object. It will replace its buffer,
sentinel and filter. In the "autostart-server" branch of eglot.el, for
example, there is a function to I start a local process with special
arguments to that it open a server, then connect to it, then return that
connected process. This is for Windows LSP servers that can't to stdio
comms.

> - I think many of the names can be internal (i.e. contain a double
> dash) unless they are explicitly part of the public API.

Maybe. What names? The names that I want to make public are already
public, and all the others aren't.

> - Probably jrpc-async-request should just return nil. Or is there a
> situation where callers have to know about IDs or timers? I think
> those should be treated as implementation details and not be exposed
> to clients.

OK. Makes sense.

> - Please use hashtables or alists instead of plists. There are many
> more functions to operate on hashtables and alists,

Maybe. Hmm, I really really don't know about this. Have you tried
cl-destructuring-bind with plists? It really is the SH&T for JSON.

> and json.c doesn't support plists.

That's the superior argument. I didn't know about json.c until very
recently. I'll play along, but can you help me rewrite jsonrpc-lambda to
something less consing perhaps?

(cl-defmacro jsonrpc-lambda (cl-lambda-list &body body)
  (declare (indent 1) (debug (sexp &rest form)))
  (let ((jsonobj (gensym "jsonrpc-lambda-elem")))
    `(lambda (,jsonobj)
       (apply (cl-function (lambda ,cl-lambda-list ,@body))
              (let (plist)
                (maphash (lambda (k v)
                           (push v plist)
                           (push (if (keywordp k) k
                                   (intern (format ":%s" k)))
                                 plist))
                         ,jsonobj)
                plist)))))

> - The HTTP-style enveloping (using HTTP headers to separate responses)
> is technically not part of JSONRPC. You could either add support for
> other enveloping methods (though I'm not aware of any), or at least
> call this out in the documentation.

OK. Yes, it's most definetly not JSONRPC. It's an LSP thing. This is the
reason I need to rework the "process" object into a proper defclass and
make generic functions that only apply this enveloping to the proper
LSPlike objects (which could/should be the default object types returned
by jsonrpc-connect).

> - I think `string-bytes' isn't guaranteed to return the number of
> UTF-8 bytes. Rather you should encode the string explicitly to a
> unibyte string using `encode-coding-string', and then use
> `no-conversion' for the process coding systems.
>
I think Eli has already answered this authoritatively.

> - Consider also sending (and parsing) a Content-Type header:
> Content-Type: application/vscode-jsonrpc; charset=utf-8.

OK.

> - I don't quite understand how `jrpc-ready-predicates' is supposed to
> be used. Are clients supposed to have some form of server-specific
> sidechannel to detect when the server is ready?

Yes that's precisely it (though it's entirely optional on the client
part).

> It might be simpler to solve this problem in Emacs core, e.g. by
> adding a timeout to `process-send-string'.

No, the "ready" here is for the application, not the transport.

I understand it's obscure for a reader, so let me explain and then this
might go into the doc.  Obviously, this came out of necessity in
eglot.el, but I think its good functionality to keep in jsonrpc.el
refactor (and it'd be really hard to implement outside it.)

The problem arises in the ordering of sending multiple asynchronous
JSONRPC requests. So, for example in LSP, it doesn't make sense to send
most requests if there are outstanding changes in the buffer that you
haven't told the server about. But because the latter can come in
through idle timers, it's useful to have this generic :deferred
mechanism: you just put in `jsonrpc-ready-predicates' a function that
returns nil if there are outstanding changes and then write code without
worrying about whether the server is ready in that particular
situation. This is better than sprinkling the code with
"send-outstanding-changes-just-to-be-sure" before you make requests.

RLS (Rust's Language Server) has a more complicated limitation, so
eglot.el puts an additional predicate there for rust-mode.el

> - I'd call the library jsonrpc.el (or json-rpc.el or jsonrpc2.el
> or...) if that name isn't already taken.

OK. I don't think it is. Done

João