bug#61350: Eglot over Tramp freezes with large project

[Michael Albinus]

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

Hi João,

> I think the patch goes in the right direction.  The force resetting of
> 'timeout' to 0, which you highlighted earlier, is odd.

Yes. But I've tried first w/o this setting, and the recipe from Thomas
still didn't work.

> +  (dolist (p (delq proc (process-list)))
> +    (when (tramp-file-name-equal-p
> +        (process-get proc 'vector) (process-get p 'vector))
> +      (setq timeout  (or timeout 0))
> +      (accept-process-output p 0 nil t)))
>
> That's because callers of 'tramp-accept-process-output' who call it with
> TIMEOUT=nil will now see their expectations violated depending on
> conditions they do not control.  You fully control these callers, so you
> may know better, but I think this patch makes more sense, because it
> keeps the contract.  

In fact, I'm even not sure that a nil timeout is still needed. This is
something from the very beginning of Tramp. Maybe I shall review the
code whether we still need a nil timeout. Because its initial idea, wait
in accept-process-output until the remote shell prompt appears, is no
longer needed, because we call tramp-accept-process-output (and
therefore accept-process-output) in a loop. This is something to be
changed later on.

A short scan over the Tramp sources shows, that there are 12 calls of
tramp-accept-process-output with a TIMEOUT of 0, and only two other
calls with a nil TIMEOUT. And these two calls are already in while loop,
so I guess it would be also OK to give them a default TIMEOUT of 0.

I will try it, but this needs lot of regression tests (I have 100+
different configurations to run tramp-tests.el).

> +  (dolist (p (delq proc (process-list)))
> +    (when (tramp-file-name-equal-p
> +        (process-get proc 'vector) (process-get p 'vector))
> +      (while (accept-process-output p 0 nil t))))
>    (with-current-buffer (process-buffer proc)
>      (let ((inhibit-read-only t)
>         last-coding-system-used
>
> This is even more "conservative" than your patch, because it deviates
> less from the current behaviour.  I've tested it (first removing the
> eglot.el workaround) with my reproduction recipe to a 100% success rate
> in 6-7 experiments (whereas without it it is 0%).

Hmm, in my experience there were still one or two blocking tests wit
Thomas' recipe, IIRC. This is because the blocking situation, data is
going through the only ssh socket for both tramp-accept-process-output
and jsonrpc, is not unblocked then.

So I would prefer to keep this (setq timeout  (or timeout 0)) for
now. And I will reviw the code, whether tramp-accept-process-output
still needs a TIMEOUT ARGUMENT as sketched above.

>> I plan to install the patch next days on master, unless you oppose.
>
> I don't, but consider my alternative.

I consider it by removing the TIMEOUT argument, if possible.

> João
>
> PS: And I also think that Eglot over Tramp does seem to have become
> faster with ControlMaster, at least in a subjective evaluation.

Of course. That is the reason behind using it.

Best regards, Michael.