comments about jsonrpc.el

comments about jsonrpc.el

[Filipp Gunbin]

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

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.

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

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)

4) jsonrpc--async-request-1 - non-local exit is missing (I saw it in
the version you posted here)

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).  Another option - maybe we can just bind
inhibit-quit to t instead of this unwind-protect with cleanup?

Filipp

Re: comments about jsonrpc.el

[João Távora]

Hi Filipp,

Filipp Gunbin <fgunbin@fastmail.fm> 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

Re: comments about jsonrpc.el

[Filipp Gunbin]

João,

On 02/06/2018 01:42 +0100, João Távora wrote:

>> 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 they are independent of each other - then yes, probably only the one
which runs.

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

But line 643 says '(setq timer (funcall make-timer)))' - isn't it what
gets into continuation?  To be clear - I mean the timer which gets into
continuation (but if we re-defer deferred action - then yes, the
deferred action's timer is "inherited").

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

I'm just not familiar with cl-defun enough, sorry.  If '&rest args'
includes 'deferred' in 'args', then ok.

Thanks!
Filipp

Re: comments about jsonrpc.el

[João Távora]

Filipp Gunbin <fgunbin@fastmail.fm> writes:

>> 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.
>
> But line 643 says '(setq timer (funcall make-timer)))' - isn't it what
> gets into continuation?  To be clear - I mean the timer which gets into
> continuation (but if we re-defer deferred action - then yes, the
> deferred action's timer is "inherited").
>
Hi Filipp,

In the newer version it doesn't say that, but even in that version I am
pretty sure that `make-timer' was a lambda that checks the `timer`
variable before actually making one.

João

BTW, it's better to use diffs to highlight source code bits, it provides
context, people do it quite extensively in this group.  Also add the git
commit hash (or if the code comes from an email, indicate the origin of
the email )

Re: comments about jsonrpc.el

[Filipp Gunbin]

Hi João,

On 10/06/2018 12:39 +0100, João Távora wrote:

> In the newer version it doesn't say that, but even in that version I am
> pretty sure that `make-timer' was a lambda that checks the `timer`
> variable before actually making one.

No, that version didn't check the `timer' var inside the lambda..

> BTW, it's better to use diffs to highlight source code bits, it provides
> context, people do it quite extensively in this group.  Also add the git
> commit hash (or if the code comes from an email, indicate the origin of
> the email )

Yep, I don't know why I didn't post the diff :-)

Thanks
Filipp

Re: comments about jsonrpc.el

[João Távora]

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

On Wed, Jun 13, 2018 at 8:09 PM, Filipp Gunbin <fgunbin@fastmail.fm> wrote:

> Hi João,
>
> On 10/06/2018 12:39 +0100, João Távora wrote:
>
> > In the newer version it doesn't say that, but even in that version I am
> > pretty sure that `make-timer' was a lambda that checks the `timer`
> > variable before actually making one.
>
> No, that version didn't check the `timer' var inside the lambda..
>

Hmmm. Well anyway it does now.  But really thanks for the
close attention.

João

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