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

[miha]

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

>> I think that accept-process-output with JUST-THIS-ONE=nil is dangerous
>> here. We are now allowing 'file-exists-p', 'expand-file-name' and all
>> other functions listed in 'tramp-sh-file-name-handler-alist' to call any
>> timer or process filter,
>
> That's exactly what we want.  We _don't_ want to stop other
> output to be inhibited from going into the filter.  We want
> to let other processes slurp their input as often as possible.

Generally, I agree. JUST-THIS-ONE=nil is better and it's fine to use it
in most functions as long as the programmer is careful that foreign
timers and proc filters won't interfere. Though I do think that if a
function that calls a-p-o with JUST-THIS-ONE=nil isn't internal, it
should mention that it can run timers in its doc string.

But here we are talking about 'expand-file-name' and other functions
from tramp-sh-file-name-handler. For these, I disagree that we want them
to accept output from other processes and call their filters.
'expand-file-name' and others are used often and, for most of the
existence of Emacs, they did not call proc filters. I think it's
dangerous to have them do it now.

> The reason it hung with JUST-THIS-ONE=t is precisely the inverse
> reason that it hangs without this hunk (say, in emacs-29).  One
> process (say tprocess) is holding its input and jprocess can't read
> it because it comes from a shared underlying SSH buffer and that
> buffer is likely full (until tprocess empties it a little bit).
>
> At the moment, it is the best we have, both theoretically and
> experimentally.
>
>> without even documenting this.
>
> This is the most normal usage of accept-process-output.
> Look at all the Emacs code base.  Basically, only Tramp uses
> JUST-THIS-ONE.  It's JUST-THIS-ONE who is the aberration.
> None  of the process-heavy SLIME, SLY, CIDER IDEs use it.
>
> You can read the manual about JUST-THIS-ONE.  It's in A-O-P.

Sorry I wasn't clear enough. I meant documenting in expand-file-name's
doc string that calling expand-file-name can now run arbitrary process
filters and timers. The same goes for other functions from
'tramp-sh-file-name-handler-alist'.

>> filter may call 'read-passwd', entering a recursive edit, in which the
>> user can kill any buffer or even a process. Or with midnight-mode
>> enabled, a timer kills buffers older than 3 days. I consider these "the
>> problem of timing errors that usually plague parallel programming", to
>> quote '(elisp) Output from Processes'.
>
> Sorry, I don't immediately see the problem.  I can't prove that there
> isn't one :-) that's near impossible.  Normally what is done
> is for the problem to be demonstrated with some experiment.
>
>> I'm not saying that my proposal with SIGWINCH is flawless and I agree
>> that its unacceptable. I'm just saying that we shouldn't be quite
>> satisfied with this solution yet, though it should be fine for some
>> time.
>
> I'm not satisfied with it myself.  This is very slow (not that it matters
> much).  But is also all around ugly.  The more correct solution is IMHO,
> described in bug#61350.  But it will take time to see if it fits into
> Tramp (I think it does) but most importantly how (that's the hard bit,
> Tramp is a lot of code)

I'm not against your way of refactoring tramp process filters but I
don't think it will solve our problems in any fundamental way for the
following simple reason:

'expand-file-name' is synchronous, therefore tramp has to make it use
accept-process-filter (tell me if there's another way). It can use it in
roughly two ways: with JUST-THIS-ONE=t or JUST-THIS-ONE=nil. In the
first case, we hit the eglot-over-ssh freeze problem and in the second
case, we'll have 'expand-file-name' call timers and process filters,
which I explained above why I don't like.

>> What I was thinking was perhaps to introduce a function called
>> 'accumulate-process-output' which would be similar to
>> 'accept-process-output', except that it would only save process output
>
> You can propose this in emacs-devel, but if you permit me to
> be slightly immodest, I've been coding with
> accept-process-output + filters for almost 15 years and I've never
> had the need for this.  I normally want timers and other processes
> to run, I don't want to stop the world.

This is great for other packages, but tramp.el carries the special
burden of having to re-implement 'expand-file-name' and other functions
of synchronous nature.

signature.asc
Description: PGP signature