Re: New jrpc.el JSONRPC library

[João Távora]

Philipp Stephani <address@hidden> writes:

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

Well, the idea was to hold the discussion here since I'm proposing this
as a core library. But I guess we can do both, why not...

Make your pull requests against the `jsonrpc-refactor' branch.

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

Have a look at the eglot-tests.el, it's testing some of these things
(crashes, reconnections, timeouts, etc...) though from an LSP
standpoint.

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

jsonrpc-events-buffer is a logging facility specially apt at printing
JSON objects and edebug is not really practical to watch sessions of
JSONRPC at a glance. I guess I can extract that stuff into
jsonrpc-debug.el, but lets make that one of the last things.

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

Yeah, but it's the API's responsibility to guide the user. Common things
easy, complicate things possible. I just made it possible.

> Please also use specific error symbols in `jsonrpc-error'.

I think I already did,  jsonrpc-error-message and jsonrpc-error-code

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

Never mind, this is going away completely once I use classes to
represent JSON endpoints.

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

It's most definitly *not* an implementation detail *when using them for
an API*, because if you change the implementation of you structure-based
library, all the code compiled against it will break. You can think of
structures as just an array with fancy accessor names that all expand to
aref's. they're only useful when confined to the same compilation unit.

iow, speed comes at a price.

For a real-life example read the comments in this emacs-lsp issue:
https://github.com/emacs-lsp/lsp-mode/pull/289

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

Auto-restarting on send isn't a spectacular idea . You should leave it
to the client to do this imo.

eglot.el, the only existing jsonrpc.el client today, does something that
is similar and that you may think is sufficient: when the process-based
server crashes, jsonrpc.el catches it in the sentinel and calls an
eglot.el hook that reconnects automatically. Obviously, this only
happens if the previous session lasted more than x seconds, to prevent
infinite reconnections.

If this doesn't suit you, in a hypothetical stephani.el client, you
should be able to do the "ensure-connected" dance without much effort
using the same jsonrpc.el API.

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

Factories are so 1995 :-) But think I understand what you want more or
less. In master eglot.el, I'm already using classes. If you don't want
the stdio/tcp process-based defaults (or if you want them with some
added twist), you can pass a class symbol to the connect function and
jsonrpc.el will makes the object with nothing but fundamental JSONRPC
stuff (hidden in the base class). There's your factory, I guess. Then
generic functions are called whenever behaviour can be customized.

Figuring out what these generic functions and where the default methods
lie will be the meat of desining the API.

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

Yeah default connection/endpoint class should probably give you a
process similar to what is used for talking LSP with clients. 
   
>  > - 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.

jsonrpc-outstanding-request-ids is an unused leftover that should be
deleted.

jsonrpc-warn is supposed is a convenience function for leting the client
signal warnings specific to jsonrpc. I see no wrong in making it public,
but I guess you can argue that clients shoudn't introduce in the jsonrpc
log at all.

Any more problems?

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

See my answers in the other thread where I go into length about this. In
short, cl-destructuring-bind gives you destructuring, validation, setting
defaults, and consistency with generic function arguments lists. 

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

Here's an example: Suppose you're implementing a spec that requires you
to iterate an array of JSON objects that

 - *must* have the key :foo;
 - *may* have the key :bar, defaulting to "baz";
 - *must not* have any other key.

I do this with:

   (mapc (jsonrpc-lambda (&key (foo nil foo-provided-p) (bar "baz"))
              (unless foo-provided-p (error "Gotta have foo!"))
              (frobnicate foo bar))
         json-objects)

Can you come up with something more concise in elisp? Come to think of
it, what about in any other language?

You could have used a cl-defmethod/cl-defun instead of the
jsonrpc-lambda anonymous function.

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

It's not a problem specific to eglot.el, but to LSP in general and other
applications that process events asynchronously but need order in their
communications.

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

It's not exactly a "global variable", but a hook variable. And it's
going to be a generic function with a default "send immediately"
behaviour. It can't easily be implemented entirely in the client, since
it requires that the client cannot tell if the request or notificaiton
sent really went through immediately.

Let me try to reword generally: a jsonrpc-client in an order-sensitive
JSONRPC-based protocol (such as LSP) *can* indicate to jsonrpc.el that
it *may* defer certain requests to the future. The decision is not
jsonrpc's to make: the client must customize an API point (used to be
the hook, now is a cl-method) that the jsonrpc.el server probes from
time to time. This enables the client to write code as if it weren't
order-sensitive at all.

João