jsonrpc.el closer to merging

jsonrpc.el closer to merging

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 2416 bytes --]

Hi,

The last few weeks, I've been getting jsonrpc.el ready for integration
into core (master/26.2) and/or ELPA.git.

There's a new version after my sig, or you can find it in the
jsonrpc-refactor branch of:

   https://github.com/joaotavora/eglot

The `;;; Commentary:' section contains an introduction to the mostly new
API and the rudiments of a future manual, as do the dostrings.

I think I have addressed most comments posted here by reviewers, to whom
I'm very grateful:

- The API library is based on a single `jsonrpc-connection' eieio class
  and some cl-generic functions;

- That class is abstract, so it's no good to use on its own, you have to
  subclass it. There is, however, a `jsonrpc-process-connection'
  transport suitable for LSP (and possibly other uses), which also
  serves as an example transport implementation.

- eglot.el (in the jsonrpc-refactor branch) is working on top of the new
  library and servers as an example application implementation, as do
  the tests in jsonrpc-tests.el;

- The library works with json.c if it is available, or json.el in its
  absence, which makes it compatible with Emacs 26.1.

  Note that if json.c is available, the library expects it to accept
  customizable null and false types in json-parse-string and
  json-serialize, which is a patch that I'm still waiting for comments
  on;

- The library exchanges JSON objects with the client as plists.  I still
  haven't gotten a very convincing argument to do so, but it could be
  made plist/alist/hashtable-agnostic. This would probably amount to
  writing a `jsonrpc--destructure-json' which autodetects the map type
  and using it instead of `cl-destructuring-bind';

- tests aren't extensive, but they do cover some tricky corner cases.
  eglot's tests are also passing on top of the new jsonrpc.el.  The
  "deferred actions" code has been cleaned up of many bugs (thanks to
  Filipp Gunbin) but perhaps it would be wise to write tests for those;

- The `jsonrpc-process-connection' class sends the Content-Type header
  along with Content-Length;

- There is no more functions
  jsonrpc-current-connection/jsonrpc-current-connection-or-lose.  There
  are also no more interactive functions.  Applications should implement
  those;
  
- There are no defcustoms;

After integration, I can write up the proper docs and NEWS entry.

João


[-- Attachment #2: jsonrpc.el --]
[-- Type: application/emacs-lisp, Size: 32625 bytes --]

Re: jsonrpc.el closer to merging

[Clément Pit-Claudel]

On 2018-06-10 11:56, João Távora wrote:
> The last few weeks, I've been getting jsonrpc.el ready for integration
> into core (master/26.2) and/or ELPA.git.

I think this is great news.  I have multiple packages that use a client-server architecture, and moving them to JSON-RPC would be a nice simplification.

It would be nice if you hosted this on ELPA, so that others could take a dependency on that package without having to wait for new releases of Emacs :)

How closely does it follow JSON-RPC? For example, does it allow for servers to send notifications to clients? Does it allow more than one response per query?  If the server is killed, what does it do with pending queries?

Thanks again for this work!
Clément.

Re: jsonrpc.el closer to merging

[João Távora]

Clément Pit-Claudel <cpitclaudel@gmail.com> writes:

> On 2018-06-10 11:56, João Távora wrote:
>> The last few weeks, I've been getting jsonrpc.el ready for integration
>> into core (master/26.2) and/or ELPA.git.
> It would be nice if you hosted this on ELPA, so that others could take
> a dependency on that package without having to wait for new releases
> of Emacs :)

Yes ELPA is my plan too.  As far as I understand, that isn't
incompatible with also having it in core.

> How closely does it follow JSON-RPC?

Pretty closely, but JSONRPC is pretty lax :-)

> For example, does it allow for servers to send notifications to
> clients?

Yes.  Both endpoints can send requests that wait for responses, and
notifications that don't.  JSONRPC doesn't distinguish between clients
and servers, though the application may do so (eglot.el does, for
example).

> Does it allow more than one response per query?

Not sure I follow.  In JSONRPC (and in most connection-oriented
protocols) a response, by definition, something that a request waits on.
Once it occurs the request is considered completed.  jsonrpc.el has two
ways to model this: jsonrpc-request, blocking and jsonrpc-async-request,
a non-blocking.

The remote endpoint can send more notifications after responding.  Or
the client can trigger more requests after it get its first response.

> If the server is killed, what does it do with pending queries?

Depends on the underlying transport.  Currently the only concrete
transport implementation, jsonrpc-process-connection, cancels all
pending queries and calls their error handler.  This means either a
blocking jsonrpc-request will exit non-locally or you will get an error
in the process sentinel.

> Thanks again for this work!

You're very welcome.

João

Re: jsonrpc.el closer to merging

[Clément Pit-Claudel]

On 2018-06-10 18:36, João Távora wrote:
> Yes.  Both endpoints can send requests that wait for responses, and
> notifications that don't.  JSONRPC doesn't distinguish between clients
> and servers, though the application may do so (eglot.el does, for
> example).

Ah, neat.  But then I'm confused. The spec you linked to (http://www.jsonrpc.org/specification) does distinguish, and there it seemed that only the client could send notifications.

> Not sure I follow.  In JSONRPC (and in most connection-oriented
> protocols) a response, by definition, something that a request waits on.
> Once it occurs the request is considered completed.  jsonrpc.el has two
> ways to model this: jsonrpc-request, blocking and jsonrpc-async-request,
> a non-blocking.
> 
> The remote endpoint can send more notifications after responding.  Or
> the client can trigger more requests after it get its first response.

Thanks. The context is that I'm trying to see what I need to change to use your library.
I have code in which the server responds with progress information as it processes a query.  For example

-> (id=xyz) Compute \pi to 15 decimals
<- (id=xyz, progress) 10% done
<- (id=xyz, progress) 60% done
<- (id=xyz, response) 3.141592653589793

The same lambda gets invoked three times, twice with 'progress and a message, and once with 'done and a number.
Is there a way to model this is json-rpc.el?

Thanks!
Clément.

Re: jsonrpc.el closer to merging

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 3345 bytes --]

[Clément, sorry if you are getting this in duplicate]

On Mon, Jun 11, 2018 at 2:08 PM, Clément Pit-Claudel <cpitclaudel@gmail.com>
 wrote:

> On 2018-06-10 18:36, João Távora wrote:
> > Yes.  Both endpoints can send requests that wait for responses, and
> > notifications that don't.  JSONRPC doesn't distinguish between clients
> > and servers, though the application may do so (eglot.el does, for
> > example).
>
> Ah, neat.  But then I'm confused. The spec you linked to (
> http://www.jsonrpc.org/specification) does distinguish, and there it
> seemed that only the client could send notifications.
>


I'm sorry, I totally confused you.  But I'm still more or less right, I
think:

    " The Client is defined as the origin of Request objects and the
    handler of Response objects.  The Server is defined as the origin of
    Response objects and the handler of Request objects.

    One implementation of this specification could easily fill both of
    those roles, even at the same time, to other different clients or
    the same client. "

jsonrpc.el is one such implementation :-)

>
> > Not sure I follow.  In JSONRPC (and in most connection-oriented
> > protocols) a response, by definition, something that a request waits on.
> > Once it occurs the request is considered completed.  jsonrpc.el has two
> > ways to model this: jsonrpc-request, blocking and jsonrpc-async-request,
> > a non-blocking.
> >
> > The remote endpoint can send more notifications after responding.  Or
> > the client can trigger more requests after it get its first response.
>
> Thanks. The context is that I'm trying to see what I need to change to use
> your library.
> I have code in which the server responds with progress information as it
> processes a query.  For example
>
> -> (id=xyz) Compute \pi to 15 decimals
> <- (id=xyz, progress) 10% done
> <- (id=xyz, progress) 60% done
> <- (id=xyz, response) 3.141592653589793
>
> The same lambda gets invoked three times, twice with 'progress and a
> message, and once with 'done and a number.
> Is there a way to model this is json-rpc.el?
>

Hmm, I see.  No the library doesn't allow this (and neither does
JSONRPC, I think)

There's two ways you can do this (in pseudocode)

(require 'cl-lib)

(let (retval
      (calculation-id (symbol-name (gensym))))
  (while
      (cl-destructuring-bind (&key progress result)
          (jsonrpc-request conn
             :compute-pi `(:id ,calculation-id :decimals 15))
        (setq retval result)
        (and progress (message "%s" progress)))))

This would make n requests and n responses and the request handler on
the other side must find the calculation id.

The other way would be much simpler and much more JSONRPC'esque would be
to issue notifications while the client is waiting for the answer:

   (jsonrpc-request conn :compuate-pi `(:decimals 15))

This blocks until the final response is gotten, but you get to handle
the progress notifications in a separate notification handler.  Is there
some ambiguous situation where you would not be able to to correlate
them to the request?

Why don't you tell me, in pseudo-code, how you would like it to work?
Perhaps we can add that as a JSONRPC extension (there's a version
that I'm not using).


João

[-- Attachment #2: Type: text/html, Size: 7319 bytes --]

Re: jsonrpc.el closer to merging

[João Távora]

João Távora <joaotavora@gmail.com> writes:

>  Thanks. The context is that I'm trying to see what I need to change to use your library.
>  I have code in which the server responds with progress information as it processes a query.  For example
>
>  -> (id=xyz) Compute \pi to 15 decimals
>  <- (id=xyz, progress) 10% done
>  <- (id=xyz, progress) 60% done
>  <- (id=xyz, response) 3.141592653589793
>
>  The same lambda gets invoked three times, twice with 'progress and a message, and once with 'done and a number.
>  Is there a way to model this is json-rpc.el?
>  
> Hmm, I see.  No the library doesn't allow this (and neither does
> JSONRPC, I think)
>
> Why don't you tell me, in pseudo-code, how you would like it to work?
> Perhaps we can add that as a JSONRPC extension (there's a version 
> that I'm not using).

I though a bit more about this and your use case seems to be quite
reasonable.

First, when I sand "there's a version", I meant "there's a version
field" that I'm not using, it's usually the string "2.0". But if we *do*
use it, we can set it to something like 2.0-EMACS which means a
backward-compatible extension to the JSONRPC protocol.

In this extension, responses can have a :PARTIAL field, and the endpoint
receiving such a request should expect more responses with that ID until
a final response with the regular :RESULT field comes in.

What do you think?

I implemented this in the 'multi-response-requests' branch of
git@github.com:joaotavora/eglot.

Here's an example usage from the automated tests file jsonrpc-tests.el:

In this fixture the server responds thrice with partials 1 2 and 3
before sending the actual results.  You can pass :partial-handler to
jsonrpc-request and it will get called with the partial result (while
the function is still blocking).

Or you can pass a more generic SUCCESS-FN to jsonrpc-async-request.

(ert-deftest partial-replies ()
  "Test some basic partial replies."
  (jsonrpc--with-emacsrpc-fixture (conn :partial-responses '(1 2 3))
    (should-error
     (= 42 (jsonrpc-request conn '+ [40 2])))
    (should
     (= 42 (jsonrpc-request conn '+ [40 2] :jsonrpc "2.0-EMACS")))
    (should
     (let ((counter 0))
       (= 42 (jsonrpc-request
              conn '+ [40 2]
              :jsonrpc "2.0-EMACS"
              :partial-handler (lambda (partial)
                                 (should (= (cl-incf counter) partial)))))))
    (catch 'done
      (let ((counter 0))
        (jsonrpc-async-request conn '+ [40 2] :jsonrpc "2.0-EMACS"
                               :success-fn
                               (lambda (result &optional partial)
                                 (when result
                                   (should (null partial))
                                   (should (= result 42))
                                   (throw 'done nil))
                                 (when partial
                                   (should (null result))
                                   (should (= (cl-incf counter) partial)))))
        ;; Need this so that the test keeps running a little bit
        (while (jsonrpc-running-p conn) (accept-process-output nil 3))))))

Re: jsonrpc.el closer to merging

[Clément Pit-Claudel]

On 2018-06-11 18:26, João Távora wrote:
> In this extension, responses can have a :PARTIAL field, and the endpoint
> receiving such a request should expect more responses with that ID until
> a final response with the regular :RESULT field comes in.
> 
> What do you think?

I think this looks great! And I love that your patch to implement this is quite small and understandable.

But I'm also wary of adding extensions to a neat & lean library like json-rpc without a rock-solid use case (and I don't think my own packages provide a solid enough use-case).  I'd be especially worried to introduce extensions like this for fear that a future version of JSON-RPC might go in a different direction.

Given that the patch is fairly simple, how tricky do you think it would be to refactor json-rpc slightly to make it possible for clients of the library to implement such an extension?  I haven't looked at the code enough to know how it would be done, but I think it would be the best of both worlds: json-rpc remains small and with well-defined scpoe, but we add enough knobs to allow clients to reuse its foundations while implementing a slightly more complex protocol.

Btw, there are some discussions on how to do this online, like https://groups.google.com/forum/#!topic/json-rpc/IqDOByU0U0Y, https://groups.google.com/forum/#!msg/json-rpc/5PcrYSfzavA/cLW5buMC48EJ, and https://groups.google.com/forum/#!msg/json-rpc/EWnUwcTmOjY/x-1_beeIJPoJ

I guess what I'm saying here is that there's a callback-tracking core in json-rpc that's useful even for protocols that are not exactly json-rpc; managing to expose it in a sufficiently flexible way (to allow building vanilla json-rpc and closely related protocols on top of it) would be wonderful, and maybe not too hard since the patch to add the particular extension I mentioned seems to have been quite small :) 

Clément.

Re: jsonrpc.el closer to merging

[João Távora]

Clément Pit-Claudel <cpitclaudel@gmail.com> writes:

> On 2018-06-11 18:26, João Távora wrote:
>> In this extension, responses can have a :PARTIAL field, and the endpoint
>> receiving such a request should expect more responses with that ID until
>> a final response with the regular :RESULT field comes in.
>> 
>> What do you think?
>
> I think this looks great! And I love that your patch to implement this
> is quite small and understandable.
>
> But I'm also wary of adding extensions to a neat & lean library like
> json-rpc without a rock-solid use case (and I don't think my own
> packages provide a solid enough use-case).  I'd be especially worried
> to introduce extensions like this for fear that a future version of
> JSON-RPC might go in a different direction.

It was just a proof of concept and it's backward compatible, but
basically you're right:  I better not focus here.

> Given that the patch is fairly simple, how tricky do you think it
> would be to refactor json-rpc slightly to make it possible for clients
> of the library to implement such an extension?  I haven't looked at
> the code enough to know how it would be done, but I think it would be
> the best of both worlds: json-rpc remains small and with well-defined
> scpoe, but we add enough knobs to allow clients to reuse its
> foundations while implementing a slightly more complex protocol.

> I guess what I'm saying here is that there's a callback-tracking core
> in json-rpc that's useful even for protocols that are not exactly
> json-rpc; managing to expose it in a sufficiently flexible way (to
> allow building vanilla json-rpc and closely related protocols on top
> of it) would be wonderful, and maybe not too hard since the patch to
> add the particular extension I mentioned seems to have been quite
> small :)

I agree.  As you know, there are also downsides to opening "too much"
API (as in you have to maintain it) and the code might become much more
convoluted than adding a single extension, though the advantages of
having something programmable should eventually surpass that.

It shouldn't be terribly hard to do.  In jsonrpc.el, the hairiest part
is jsonrpc-connection-receive and jsonrpc-connection-send, where the
:partial keyword arg needs to be passed to the function, which has a
destructuring lambda list that would currently forbid it (since it falls
outside JSONRPC) That needs to be changed in such a way that without the
extension it is still forbidden, but with it, it isn't.  Also, in
jsonrpc-connection-receive, the last part (where the continuation is
called) would have to be customizable by hook of by generic function.

Finally, in your extension pacakge just add a custom
my-jsonrpc-partial-accepting-request helper that calls
jsonrpc-async-request with a 2-parameter modified callback lambda.

So, since you seem to have grasped the gist of the code, send me a patch
for something like this :)

João

Re: jsonrpc.el closer to merging

[Stefan Monnier]

> - The library exchanges JSON objects with the client as plists.  I still
>   haven't gotten a very convincing argument to do so, but it could be
>   made plist/alist/hashtable-agnostic. This would probably amount to

I'm not sure I understand this: why would one want to make it
p/a/h-agnostic?


        Stefan

Re: jsonrpc.el closer to merging

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 528 bytes --]

On Mon, Jun 11, 2018, 02:46 Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> > - The library exchanges JSON objects with the client as plists.  I still
> >   haven't gotten a very convincing argument to do so, but it could be
> >   made plist/alist/hashtable-agnostic. This would probably amount to
>
> I'm not sure I understand this: why would one want to make it
> p/a/h-agnostic?


So that the application that you build on top of it can use whatever. Are
you sure you aren't you confusing jsonrpc with LSP at this point?

[-- Attachment #2: Type: text/html, Size: 890 bytes --]

Re: jsonrpc.el closer to merging

[Stefan Monnier]

>> > - The library exchanges JSON objects with the client as plists.  I still
>> >   haven't gotten a very convincing argument to do so, but it could be
>> >   made plist/alist/hashtable-agnostic. This would probably amount to
>> I'm not sure I understand this: why would one want to make it
>> p/a/h-agnostic?
> So that the application that you build on top of it can use whatever. Are
> you sure you aren't you confusing jsonrpc with LSP at this point?

Oops, yes, sorry,


        Stefan