unofficial mirror of emacs-devel@gnu.org 
 help / color / mirror / code / Atom feed
* Tramp and timers
@ 2020-12-11 16:58 Michael Albinus
  2020-12-13 17:43 ` Stefan Monnier
  0 siblings, 1 reply; 5+ messages in thread
From: Michael Albinus @ 2020-12-11 16:58 UTC (permalink / raw)
  To: emacs-devel

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.



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: Tramp and timers
  2020-12-11 16:58 Tramp and timers Michael Albinus
@ 2020-12-13 17:43 ` Stefan Monnier
  2020-12-13 18:37   ` Michael Albinus
  0 siblings, 1 reply; 5+ messages in thread
From: Stefan Monnier @ 2020-12-13 17:43 UTC (permalink / raw)
  To: Michael Albinus; +Cc: emacs-devel

> 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




^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: Tramp and timers
  2020-12-13 17:43 ` Stefan Monnier
@ 2020-12-13 18:37   ` Michael Albinus
  2020-12-13 20:55     ` Stefan Monnier
  0 siblings, 1 reply; 5+ messages in thread
From: Michael Albinus @ 2020-12-13 18:37 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel

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.



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: Tramp and timers
  2020-12-13 18:37   ` Michael Albinus
@ 2020-12-13 20:55     ` Stefan Monnier
  2020-12-14  7:55       ` Michael Albinus
  0 siblings, 1 reply; 5+ messages in thread
From: Stefan Monnier @ 2020-12-13 20:55 UTC (permalink / raw)
  To: Michael Albinus; +Cc: emacs-devel

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




^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: Tramp and timers
  2020-12-13 20:55     ` Stefan Monnier
@ 2020-12-14  7:55       ` Michael Albinus
  0 siblings, 0 replies; 5+ messages in thread
From: Michael Albinus @ 2020-12-14  7:55 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel

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.



^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-12-14  7:55 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-12-11 16:58 Tramp and timers Michael Albinus
2020-12-13 17:43 ` Stefan Monnier
2020-12-13 18:37   ` Michael Albinus
2020-12-13 20:55     ` Stefan Monnier
2020-12-14  7:55       ` Michael Albinus

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).