bug#62194: 30.0.50; Two Eglot-over-Tramp tests are failing on master, passing on emacs-29

[Michael Albinus <michael.albinus@gmx.de>]

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

Hi João,

>> So I propose we let the state as-it-is in master. The relevant tests
>> pass successfully, and we have your workaround in eglot.el, which makes
>> the situation a little bit better.
>
> I propose we still revert the two of your latest patches.  The situation
> stays exactly the same in practice for now (bug#31350 broken, this bug
> fixed, workaround in place, no-one knows what is really happening), but
> with the added advantage that the code is still the same as in emacs-29.

Nope, I'd like to keep the changes. It is the basis for further work on
the problem. And these changes will also be in Tramp 2.6.0.3.

>> Just one remark: perhaps you could
>> change this to
>>
>> --8<---------------cut here---------------start------------->8---
>>                         (let ((default-directory default-directory)
>>                               ;; bug#61350: Tramp turns on a feature
>>                               ;; by default that can't (yet) handle
>>                               ;; very much data so we turn it off
>>                               ;; unconditionally -- just for our
>>                               ;; process.
>>                               (tramp-use-ssh-controlmaster-options 'suppress)
>>                               (tramp-ssh-controlmaster-options
>>                                "-o ControlMaster=no -o ControlPath=none"))
>> --8<---------------cut here---------------end--------------->8---
>
>
>> For the Tramp < 2.6.0.3 it still works, because
>> tramp-use-ssh-controlmaster-options is non-nil, and
>> tramp-ssh-controlmaster-options is used. Starting with Tramp 2.6.0.3,
>> the value `suppress' forces Tramp to compute its own
>> tramp-ssh-controlmaster-options, which might be the same, or not. But it
>> is Tramp's responsibility to DTRT.
>
> Makes sense.  If it's just this change, you can push this yourself.
> Thanks in advance.

Pushed to the emacs-29 branch.

>> Note that I have plans to enable shared connections also for PuTTY, by a
>> similar option tramp-use-shared-connection (or similar, not decided
>> yet). But this will be relevant for MS Windows users only; I don't know
>> how many of them use eglot. And it will definitively be in Tramp 2.7
>> only.
>>
>> As proposed. we shall close *this* bug. The reported problem is fixed,
>> and for everything else we have bug#61350.
>
> OK.
>
> Just a heads up, I asked for bug#61350 and this one to be "merged"
> earlier.  Don't know what debbugs did about that, but didn't see any
> practical effect

This are two different bugs. Bug#61350 is about using a shared
connection, and this is still open, although mitigated by Eglot's
workaround.

Bug#62194 is about a patch which tried to fix bug#61350. This patch has
been reverted, and so there's nothing left to do for bug#62194. It can
be closed.

>> > FWIW, removing the JUST-THIS-ONE make Thomas' example always pass, but
>> > it has other implications like the re-entrancy thing, which I don't
>> > understand.
>> >
>> > I don't have any better ideas at the moment, other than just biting the
>> > bullet and reading Tramp's code very closely.  I'll try my hand at
>> > adapting a process-filter into it as I described in bug#61350, but I
>> > don't know if I'll manage of course, since I'm not closely acquainted
>> > with the API.
>>
>> I will continue to bring threads into play with Tramp, again. Slow
>> progress only. But perhaps, it helps to improve the situation.
>
> I think you should consider bring stuff _out_ of Tramp instead of _in_.
>
> Consider removing tramp-a-o-p entirely, and segregating/segmenting
> messages in a process filter.  This segregation is entirely textual
> (no fs primitives) and does run the risk of reentrancy.  Then -- for
> sync APIs -- 'throw' the complete message into whoever is blockingly
> waiting for the answer with (catch ... (while (accept-process-output p))).
>
> I've given a working example in bug#61350.  If you need timeouts
> I can show you how to add them.

The problem is even more complex than discussed so far. All of
asynchronity can happen to Tramp's reading of process output. Process
filters and process sentinels from other processes, timers, callbacks
from file notifications or D-Bus events, whatever. I'm not convinced
(yet?), that moving Tramp's process output reading to a process filter
will solve it.

There is also the additional problem in Emacs, that process filters do
not cascade. Any package can activate an own process filter, even for
Tramp processes, and Tramp would be lost then immediately.

> João

Best regards, Michael.