emacs-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: New jrpc.el JSONRPC library


From: Philipp Stephani
Subject: Re: New jrpc.el JSONRPC library
Date: Thu, 24 May 2018 12:02:33 +0200

Thanks!

João Távora <address@hidden> schrieb am So., 20. Mai 2018 um 17:44 Uhr:
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

Thanks! Could you maybe use GitHub PRs for further changes? I find it much easier to comment on them instead of via mail.
 


> - 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.

It's OK to add the tests once the API stabilizes. It would be great to have tests e.g. for
- server process crashes
- server takes too long to respond
- invalid responses
- mismatched IDs
- very large responses
etc., just the usual failure modes.
 

> - 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".

When reading the code, I was a bit confused to see the notion of a "current process". Later, I realized that it's only used for some debug helpers and not for the core API.
I think it would make sense to separate the debug helpers from the core API (connect, send, receive) more cleanly. I'm even wondering whether the debug helpers are needed at all. Probably you saw a need for them, otherwise you wouldn't have added them. But maybe at least in the beginning Emacs's builtin debug functionality (edebug etc.) is already good enough? If at all, please separate the debug helpers from the main API (with a comment block or similar) and move them to the end, as they are not essential.
 

> - 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.

When designing a general-purpose API, it's often unknown how it will be used; often it's used in unpredicted and surprising way. Even if we don't see a good use case to catch JSON-RPC errors now, it's quite likely that such a use case will show up in the future once the library is more widely used, and then the very minor cost of defining specific error symbols pays off.
Please also use specific error symbols in `jsonrpc-error'.
 

> - `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.

I'm not so much opposed to structures, it's more of an implementation detail and a matter of taste whether you prefer structures or classes.
However, there's a pretty strong argument that I forgot about: for robustness you need a way to restart process and even have them restarted automatically. Let's say a server process crashes or the user kills it; you definitely don't want LSP/JSON-RPC to be unusable until the user restarts Emacs. Therefore, instead of having the 'connect' function create the process, the send and receive functions should ensure that it's running; i.e. shift the paradigm from "edge-triggered" (process is created once the user calls a function) to "level-triggered" (all functions that need a process make sure the process is available). That is at the same time more robust and easier to understand. However, it practically requires using a structure or class as main data type, because the process object needs to be replaced after creating the client.
 

> - 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.

It's good to have such an extensibility mechanism, and I indeed overlooked that you can pass a process object to the connect function.
I'm wondering whether that should indeed be the *only* option: leave the process creation business entirely to the client, and only accept a process object (or rather, process factory function, because you want to restart processes as necessary, see above). That is, have the following interface instead of the 'connect':

(defun jsonrpc-make-client (process-factory)
  "PROCESS-FACTORY gets called an unspecified number of times and has to return a new live process object each time it's called."
  ...)

Optionally you could provide convenience functions to return such factories for `make-process' etc., but you could also leave that to the users entirely.
 

> - 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.

I'm thinking about things like jsonrpc-warn, jsonrpc-outstanding-request-ids, etc. These seem like implementation details that clients shouldn't care about.
 

> - 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.

Not really, how would it work? Typically I use plain gethash, assq, etc. Or `let-alist'.
 

> 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)))))

Why do we need jsonrpc-lambda? Writing a normal lambda and some gethash or assq calls doesn't sound overly bad to me. I'd just remove jsonrpc-lambda entirely; it's the kind of syntactic sugar that I think doesn't make code significantly more readable.
 

> - 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.)

If it should stay in jsonrpc.el, then it should be clear without referring to eglot.el.
 

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.

Hmm. I need to think a bit more about this. I guess it makes sense, but please make it a property of the client, not a global variable for now. 

reply via email to

[Prev in Thread] Current Thread [Next in Thread]