Tramp and timers

Tramp and timers

[Michael Albinus]

Hi,

Tramp is plagued since ever with timers, which run while Tramp is
performing accept-process-output. If such a timer runs an operation
which includes also a remote file operation, it ruins Tramp's current
process handling. See the discussion at
<https://lists.gnu.org/archive/html/tramp-devel/2020-12/msg00003.html>
for a recent problem.

In order to fix this, I plan to push a patch, which let Tramp detect
this kind of situation. If Tramp is performing an atomic process
operation (process-send-string / accept-process-output sequence), it
creates an internal lock. If another remote file operation tries to
perform a similar atomic process operation, while Tramp is locked, an
error will be raised.

This should happen only for remote file operations called from
timers. It does not worsen the current situation; instead of blocking
whole Emacs we'll get an error which can be handled. The timer is
cancelled, but the normal Emacs operations still continue.

(Maybe there are similar problems with process sentinels, but I do not
recall a respective bug report)

People writing timer functions shall care, at least by wrapping such
calls with ignore-errors. Currently, a file-error is raised, but we
could also introduce a new error symbol.

Comments?

Best regards, Michael.

Re: Tramp and timers

[Stefan Monnier]

> Tramp is plagued since ever with timers, which run while Tramp is
> performing accept-process-output. If such a timer runs an operation
> which includes also a remote file operation, it ruins Tramp's current
> process handling. See the discussion at
> <https://lists.gnu.org/archive/html/tramp-devel/2020-12/msg00003.html>
> for a recent problem.

Here's my take on it (I'm not familiar enough with the code to be sure
it's workable, so it's more like a backseat driver's design, so feel
free to ignore):

I think it should be OK for the timer to access a Tramp file on host
A while we're in the middle of a Tramp access to a file on host B.
So the interference should be detected "per connection" rather than
globally.

Every connection should have a lock.  Whenever the connection is "in
use", we take the lock.  This is also useful in the case of the use
of threads.  When we try to take the lock and it's already taken by
another thread, we should just wait (or rather, the lock should do that
for us), but if it's already taken by the current thread then we should
instead signal an error like `tramp-recursive-access`.

Timers should arguably each run in their own thread.  Of course, they
(currently) don't, but if a timer function wants to use Tramp files,
then it should start by delegating its job to a thread (we should
probably have a standard function like `run-with-timer-concurrent` which
calls the timer function in a separate thread).  If they don't, then the
risk getting `tramp-recursive-access`.

IIUC your proposal is quite similar to what I describe,


        Stefan

Re: Tramp and timers

[Michael Albinus]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

Hi Stefan,

> I think it should be OK for the timer to access a Tramp file on host
> A while we're in the middle of a Tramp access to a file on host B.
> So the interference should be detected "per connection" rather than
> globally.

Exactly.

> Every connection should have a lock.  Whenever the connection is "in
> use", we take the lock.

That's how I have implemented it. The lock is kept per (connection)
process object.

> This is also useful in the case of the use of threads.  When we try to
> take the lock and it's already taken by another thread, we should just
> wait (or rather, the lock should do that for us), but if it's already
> taken by the current thread then we should instead signal an error
> like `tramp-recursive-access`.

Locally, I have a half baken implementation for Tramp using threads. It
is stalled for the known reasons. When I will continue, I will care
about.

> Timers should arguably each run in their own thread.  Of course, they
> (currently) don't, but if a timer function wants to use Tramp files,
> then it should start by delegating its job to a thread (we should
> probably have a standard function like `run-with-timer-concurrent` which
> calls the timer function in a separate thread).

Good idea, I haven't thought about this. And maybe I can make this
already working now, because timers using Tramp shouldn't suffer from
the problems which have stalled current implementation, mainly
interactive I/O in different threads. Timers shall not interact with the
user.

Another idea for using threads in Tramp is 'vc-registered'. It is a
magic function, and Tramp has an own implementation.

> If they don't, then the risk getting `tramp-recursive-access`.

In my current implementation, I have declared a new standard error
'remote-file-error'. I don't believe, that the term "tramp" shall be
part of the name. And I do plan to use it also for other internal error
situations, that's why it shouldn't be restricted to
"recursive-access". Maybe this new standard error could also accumulate
the existing 'ftp-error' standard error symbol. At least, 'ftp-error'
shall have 'remote-file-error' as error condition.

> IIUC your proposal is quite similar to what I describe,

Yes. My implementation passes already the default test suite (a simple
"make -C test tramp-tests", and it even passes the notorious
'tramp-test43-asynchronous-requests' case - a test, which has been
disabled for years, because I couldn't bring it to fly :-)

I'll continue with regression tests (the "adb" method is hairy); I hope
to commit this later this week.

>         Stefan

Thanks for your review, and best regards, Michael.

Re: Tramp and timers

[Stefan Monnier]

>> Every connection should have a lock.  Whenever the connection is "in
>> use", we take the lock.
> That's how I have implemented it. The lock is kept per (connection)
> process object.

Great.

>> If they don't, then the risk getting `tramp-recursive-access`.
> In my current implementation, I have declared a new standard error
> 'remote-file-error'. I don't believe, that the term "tramp" shall be
> part of the name.  And I do plan to use it also for other internal error
> situations, that's why it shouldn't be restricted to
> "recursive-access".

Either way is fine by me, but of course we can have it both ways:

   (define-error 'tramp-recursive-access "Oops" 'remote-file-error)


-- Stefan

Re: Tramp and timers

[Michael Albinus]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Either way is fine by me, but of course we can have it both ways:
>
>    (define-error 'tramp-recursive-access "Oops" 'remote-file-error)

That's what my patch contains already for the 'ftp-error' symbol. Except
the "Oops" :-)

> -- Stefan

Best regards, Michael.