bug#29735: 27.0.50; It must be possible to suspend all timers

bug#29735: 27.0.50; It must be possible to suspend all timers

[Michael Albinus]

In Tramp's start-file-process implementations, there are code segments
which must be guaranteed not to be interrupted by timers. Tramp
implements this as

(let (timer-list timer-idle-list) ...)

but this is just a hack. Inside these code segments, it must still be
possible for Tramp to activate own timers, in order to handle process
output from remote side robustly.


In GNU Emacs 27.0.50 (build 41, x86_64-pc-linux-gnu, GTK+ Version 3.22.25)
 of 2017-12-16 built on detlef
Repository revision: f63d9f86b5688ac84ec6e7eecdbb6cac103dbcf2
Windowing system distributor 'The X.Org Foundation', version 11.0.11905000
System Description: Ubuntu 17.10

Recent messages:
Opening nntp server on news...done
Opening connection to imap.gmx.net via tls...
Opening connection to imap.gmx.net...done
Opening connection to outlook.office365.com via tls...
Reading active file via nnml...
Reading incoming mail from pop...
nnml: Reading incoming mail (no new mail)...done
Reading active file via nnml...done
Reading active file via nndraft...done
Checking new news...done

Configured using:
 'configure --with-file-notification=inotify'

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS GSETTINGS NOTIFY
ACL LIBSELINUX GNUTLS LIBXML2 FREETYPE M17N_FLT LIBOTF XFT ZLIB
TOOLKIT_SCROLL_BARS GTK3 X11 LCMS2

Important settings:
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: @im=none
  locale-coding-system: utf-8

Major mode: Group

Minor modes in effect:
  gnus-undo-mode: t
  erc-notify-mode: t
  erc-notifications-mode: t
  display-time-mode: t
  shell-dirtrack-mode: t
  icomplete-mode: t
  show-paren-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  buffer-read-only: t
  column-number-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
/home/albinus/src/elpa/packages/debbugs/debbugs-org hides /home/albinus/.emacs.d/elpa/debbugs-0.14/debbugs-org
/home/albinus/src/elpa/packages/debbugs/debbugs-gnu hides /home/albinus/.emacs.d/elpa/debbugs-0.14/debbugs-gnu
/home/albinus/src/elpa/packages/debbugs/debbugs hides /home/albinus/.emacs.d/elpa/debbugs-0.14/debbugs
/home/albinus/src/elpa/packages/debbugs/debbugs-autoloads hides /home/albinus/.emacs.d/elpa/debbugs-0.14/debbugs-autoloads
/home/albinus/src/elpa/packages/debbugs/debbugs-pkg hides /home/albinus/.emacs.d/elpa/debbugs-0.14/debbugs-pkg
/home/albinus/src/elpa/packages/debbugs/debbugs-browse hides /home/albinus/.emacs.d/elpa/debbugs-0.14/debbugs-browse
/home/albinus/src/elpa/packages/tramp-theme/tramp-theme hides /home/albinus/.emacs.d/elpa/tramp-theme-0.2/tramp-theme
/home/albinus/src/elpa/packages/tramp-theme/tramp-theme-autoloads hides /home/albinus/.emacs.d/elpa/tramp-theme-0.2/tramp-theme-autoloads
/home/albinus/src/elpa/packages/tramp-theme/tramp-theme-pkg hides /home/albinus/.emacs.d/elpa/tramp-theme-0.2/tramp-theme-pkg
/home/albinus/.emacs.d/elpa/telepathy-20131209.458/telepathy hides ~/lisp/telepathy
~/src/tramp/lisp/tramp-smb hides /usr/local/src/emacs/lisp/net/tramp-smb
~/src/tramp/lisp/tramp-uu hides /usr/local/src/emacs/lisp/net/tramp-uu
~/src/tramp/lisp/tramp-adb hides /usr/local/src/emacs/lisp/net/tramp-adb
~/src/tramp/lisp/tramp-archive hides /usr/local/src/emacs/lisp/net/tramp-archive
~/src/tramp/lisp/tramp-cmds hides /usr/local/src/emacs/lisp/net/tramp-cmds
~/src/tramp/lisp/tramp-cache hides /usr/local/src/emacs/lisp/net/tramp-cache
~/src/tramp/lisp/trampver hides /usr/local/src/emacs/lisp/net/trampver
~/src/tramp/lisp/tramp-ftp hides /usr/local/src/emacs/lisp/net/tramp-ftp
~/src/tramp/lisp/tramp-sh hides /usr/local/src/emacs/lisp/net/tramp-sh
~/src/tramp/lisp/tramp hides /usr/local/src/emacs/lisp/net/tramp
~/src/tramp/lisp/tramp-loaddefs hides /usr/local/src/emacs/lisp/net/tramp-loaddefs
~/lisp/dbus hides /usr/local/src/emacs/lisp/net/dbus
~/src/tramp/lisp/tramp-gvfs hides /usr/local/src/emacs/lisp/net/tramp-gvfs
~/src/tramp/lisp/tramp-compat hides /usr/local/src/emacs/lisp/net/tramp-compat

Features:
(shadow sort mail-extr warnings emacsbug pop3 utf-7 nndraft nnmh nnml
network-stream nsm starttls gnus-agent gnus-srvr gnus-score score-mode
nnvirtual gnus-msg gnus-art mm-uu mml2015 mm-view mml-smime smime dig
mailcap gnus-cache gnus-sum time-stamp nnnil smtpmail sendmail
gnus-demon nntp gnus-group gnus-undo gnus-start gnus-cloud nnimap nnmail
mail-source tls gnutls utf7 netrc nnoo gnus-spec gnus-int gnus-range
message rmc puny rfc822 mml mml-sec epa derived epg mm-decode mm-bodies
mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader gnus-win
gnus nnheader gnus-util rmail rmail-loaddefs rfc2047 rfc2045 ietf-drums
mail-utils mm-util mail-prsvr wid-edit elec-pair erc-notify erc-networks
erc-desktop-notifications erc-match notifications dbus xml erc-goodies
erc erc-backend erc-compat thingatpt pp erc-loaddefs cperl-mode
tramp-theme em-dirs esh-var esh-io esh-cmd esh-opt esh-ext esh-proc
esh-arg esh-groups eshell esh-module esh-mode esh-util finder-inf rx
docker-tramp tramp-cache slime-autoloads vagrant-tramp dash term
disp-table ehelp info package easymenu epg-config url-handlers url-parse
url-vars time tramp-sh tramp tramp-compat tramp-loaddefs trampver
ucs-normalize shell pcomplete comint ansi-color ring parse-time
format-spec advice auth-source cl-seq eieio eieio-core cl-macs
eieio-loaddefs password-cache ido seq byte-opt gv bytecomp byte-compile
cconv jka-compr icomplete paren vc cl-loaddefs cl-lib vc-dispatcher
dired dired-loaddefs time-date mule-util tooltip eldoc electric uniquify
ediff-hook vc-hooks lisp-float-type mwheel term/x-win x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode elisp-mode lisp-mode
prog-mode register page menu-bar rfn-eshadow isearch timer select
scroll-bar mouse jit-lock font-lock syntax facemenu font-core
term/tty-colors frame cl-generic cham georgian utf-8-lang misc-lang
vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms cp51932
hebrew greek romanian slovak czech european ethiopic indian cyrillic
chinese composite charscript charprop case-table epa-hook jka-cmpr-hook
help simple abbrev obarray minibuffer cl-preloaded nadvice loaddefs
button faces cus-face macroexp files text-properties overlay sha1 md5
base64 format env code-pages mule custom widget hashtable-print-readable
backquote dbusbind inotify lcms2 dynamic-setting system-font-setting
font-render-setting move-toolbar gtk x-toolkit x multi-tty
make-network-process emacs)

Memory information:
((conses 16 441838 21208)
 (symbols 48 40807 7)
 (miscs 40 76 245)
 (strings 32 102294 5183)
 (string-bytes 1 3103750)
 (vectors 16 59999)
 (vector-slots 8 1042537 16452)
 (floats 8 259 295)
 (intervals 56 361 0)
 (buffers 992 23))

bug#29735: 27.0.50; It must be possible to suspend all timers

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Date: Sat, 16 Dec 2017 16:31:23 +0100
> 
> In Tramp's start-file-process implementations, there are code segments
> which must be guaranteed not to be interrupted by timers.

Can you explain why is that?

bug#29735: 27.0.50; It must be possible to suspend all timers

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

>> In Tramp's start-file-process implementations, there are code segments
>> which must be guaranteed not to be interrupted by timers.
>
> Can you explain why is that?

Tramp must handle several asynchronous processes in parallel for a given
remote host. The main process is the working horse, sending all the remote
commands for the several file operations, and interpreting the received
output. This is performed via the connection buffer *tramp/method host*.

When another asynchronous process is needed, for example due to the call
of `start-file-process', Tramp starts that asynchronous process calling
ssh (for example). After that, initialization happens, including
password handling, setting the remote shell, cd'ing to the working
directory, calling the indicated command, and so on. During that time,
Tramp is instructed to use another process buffer. See for example
`tramp-sh-handle-start-file-process', where you find the lines

      ;; Set the new process properties.
      (tramp-set-connection-property v "process-name" name)
      (tramp-set-connection-property v "process-buffer" buffer)

All further low level process communication functions use from now on
that process. Until the initialization work is done, and the settings
are set back:

	  (tramp-flush-connection-property v "process-name")
	  (tramp-flush-connection-property v "process-buffer"))))))

If during that time a timer starts, which wants to apply a regular file
operation (let's say `file-attributes'), the corresponding commands are
sent to the process related to the just started asynchronous process,
instead to the working horse *tramp/method host*. This fails, of
course. Therefore, the start of timers between the both code samples
must be suppressed.

Best regards, Michael.

bug#29735: 27.0.50; It must be possible to suspend all timers

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: 29735@debbugs.gnu.org
> Date: Sun, 17 Dec 2017 10:08:01 +0100
> 
> When another asynchronous process is needed, for example due to the call
> of `start-file-process', Tramp starts that asynchronous process calling
> ssh (for example). After that, initialization happens, including
> password handling, setting the remote shell, cd'ing to the working
> directory, calling the indicated command, and so on. During that time,
> Tramp is instructed to use another process buffer. See for example
> `tramp-sh-handle-start-file-process', where you find the lines
> 
>       ;; Set the new process properties.
>       (tramp-set-connection-property v "process-name" name)
>       (tramp-set-connection-property v "process-buffer" buffer)

Not sure I follow: are you changing process-buffer of a process, or do
you have more than one process sharing the same process-buffer?  Or
something else?

I guess I don't know how to interpret "Tramp is instructed to use
another process buffer".  Who in this context is "Tramp", if there are
multiple async processes involved, each one with its own buffer?

bug#29735: 27.0.50; It must be possible to suspend all timers

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

>> When another asynchronous process is needed, for example due to the call
>> of `start-file-process', Tramp starts that asynchronous process calling
>> ssh (for example). After that, initialization happens, including
>> password handling, setting the remote shell, cd'ing to the working
>> directory, calling the indicated command, and so on. During that time,
>> Tramp is instructed to use another process buffer. See for example
>> `tramp-sh-handle-start-file-process', where you find the lines
>> 
>>       ;; Set the new process properties.
>>       (tramp-set-connection-property v "process-name" name)
>>       (tramp-set-connection-property v "process-buffer" buffer)
>
> Not sure I follow: are you changing process-buffer of a process, or do
> you have more than one process sharing the same process-buffer?  Or
> something else?

Several processes, every one owns a separate process buffer. But the
basic Tramp operations don't know which process is involved, therefore
the actual process is kept via the process connections "process-name"
and "process-buffer".

> I guess I don't know how to interpret "Tramp is instructed to use
> another process buffer".  Who in this context is "Tramp", if there are
> multiple async processes involved, each one with its own buffer?

"Tramp" means the low level functions which communicate with
processes. Something like `tramp-send-command', `tramp-send-string' or
`tramp-maybe-open-connection'.

Best regards, Michael.

bug#29735: 27.0.50; It must be possible to suspend all timers

[Stefan Monnier]

> If during that time a timer starts, which wants to apply a regular file
> operation (let's say `file-attributes'), the corresponding commands are
> sent to the process related to the just started asynchronous process,
> instead to the working horse *tramp/method host*. This fails, of
> course. Therefore, the start of timers between the both code samples
> must be suppressed.

Hmm... but IIUC the same problem shows up if some random process-filter
or process-sentinel uses, say, file-attributes on that same host, right?
So it's not specific to timers?

From the description you give, I understand that:
- start-file-process causes the creation of a new underlying ssh process
  (that makes sense).
- so from then on, we have 2 (or more) ssh processes on the same host
  and the issue is to know which process to use when.
So the problem is to somehow get the "context" of a given call to Tramp,
so as to know which process to use.
Do I understand correctly?

Currently you store which process to use as a "connection-property"
(and it defaults to the "main" process), so basically the "context" is
store in a kind of global variable.

Would it make sense to try and pass that "context" information as
additional arguments instead?  Or via dynamically-coped variable?

E.g. any call to file-attributes (or any other file-name-operation)
should always use the main process, right?  So the mapping from
connection->process could be stored in a dynamically-scoped var, and
tramp-file-name-handler could let-bind this var to nil?


        Stefan

bug#29735: 27.0.50; It must be possible to suspend all timers

[Michael Albinus]

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

Hi Stefan,

>> If during that time a timer starts, which wants to apply a regular file
>> operation (let's say `file-attributes'), the corresponding commands are
>> sent to the process related to the just started asynchronous process,
>> instead to the working horse *tramp/method host*. This fails, of
>> course. Therefore, the start of timers between the both code samples
>> must be suppressed.
>
> Hmm... but IIUC the same problem shows up if some random process-filter
> or process-sentinel uses, say, file-attributes on that same host, right?
> So it's not specific to timers?

In theory, yes. But I haven't seen it yet. During the initialization
process of Tramp's asynchronous processes, in
`tramp-sh-handle-start-file-process', no process-sentinel or
process-filter shall run. Tramp itself tries to avoid this, by calling
(accept-process-output proc timeout nil 0)
See `tramp-accept-process-output'.

The other situations process output could arrive are `sit-for' and
`sleep-for'. I'm not aware that these functions are called inside the
process initialization of `tramp-sh-handle-start-file-process'.

> From the description you give, I understand that:
> - start-file-process causes the creation of a new underlying ssh process
>   (that makes sense).

yes

> - so from then on, we have 2 (or more) ssh processes on the same host
>   and the issue is to know which process to use when.

yes

> So the problem is to somehow get the "context" of a given call to Tramp,
> so as to know which process to use.
> Do I understand correctly?

yes

> Currently you store which process to use as a "connection-property"
> (and it defaults to the "main" process), so basically the "context" is
> store in a kind of global variable.

yes

> Would it make sense to try and pass that "context" information as
> additional arguments instead?  Or via dynamically-coped variable?
>
> E.g. any call to file-attributes (or any other file-name-operation)
> should always use the main process, right?  So the mapping from
> connection->process could be stored in a dynamically-scoped var, and
> tramp-file-name-handler could let-bind this var to nil?

That's exactly what I've tried prior the current implementation.
`tramp-file-name-handler' is the main door all file name handler
operations must pass. Inside this, I've stored the setting of the
process connection-property somewhere, and I've set it to the "main
process". After the respective handler function returned, I've restored
the process connection property to its saved value.

Unfortunately, this is not sufficient. I've still seen errors in
`tramp-test41-asynchronous-requests' from time to time. And as I said
already, it is almost impossible to debug this. It happens rarely only,
and debugging changes time conditions.

>         Stefan

Best regards, Michael.