Re: comments about jsonrpc.el

[João Távora]

Hi Filipp,

Filipp Gunbin <address@hidden> writes:

> I was reading the last jsonrpc.el from jsonrpc-refactor branch in eglot
> repo and got few comments.  Sorry if I'm misreading something.

Great, thanks for the review.

> 1) When deferred action times out, the timer clears continuations and
> calls timeout-fn.  I think it should also clear deferred actions, so
> they are not called from next jsonrpc--connection-receive afterwards.

That's true, good catch. . I need to write some deferred 

> 2) When deferred action runs, it invokes jsonrpc-async-request which
> invokes jsonrpc-connection-send.  I think we should also clear
> deferred actions here (why leave them?).

You mean *all* deferred actions? I believe the right thing to do is to
clear the one that is about to run, since the other ones may be
subject to other conditions.

(if (jsonrpc-connection-ready-p connection deferred)
    ;; Server is ready, we jump below and send it immediately.
    (remhash (list deferred buf) (jsonrpc--deferred-actions connection))

> Also, there seem to be two ways to handle timer: a) take timer from
> deferred action and save it in continuation - then its timeout spans
> both 'wait until server ready' interval and actual response wait
> interval; b) cancel deferred action timer and create new timer - it's
> done in the current code.

No, it is (a) that is done, because we store the timer as the second
element in (FN TIMER ID) pointed to by the deferred action. And we don't
re-create the timer.

> 3) When deferred action runs, jsonrpc--async-request-1 should be
> called with 'deferred' non-nil - so it actually sends request.  But
> it's unclear to me how this deferred=t is passed there ('apply' call
> on line 621)

A call to jsonrpc--async-request-1 that defers itself is always ends up
being called again with the very same arguments. It is the
jsonrpc-connection-ready-p predicate that decides it the action runs.
>
> 4) jsonrpc--async-request-1 - non-local exit is missing (I saw it in
> the version you posted here)

Good catch. Fixed. I botch up the merges from eglot.el sometimes, and
the automated tests are catching this yet.

> 5) jsonrpc-request: there's unwind-protect with some cleanup.  Seems
> that we also need to clean deferred actions and cancel timer (that is
> in continuation).

You're right, we need that too.

> Another option - maybe we can just bind inhibit-quit to t instead of
> this unwind-protect with cleanup?
>
No, I think this is a bad idea, let's let the user quit a request if he
wants.

João