bug#54606: 27.1; next/previous image command in image-mode triggers tramp connections for all remote dired buffers

bug#54606: 27.1; next/previous image command in image-mode triggers tramp connections for all remote dired buffers

[Phil Sainty]

In 27.1 this code is in `image-next-file'.  In master it's now
`image-mode--directory-buffers', but the relevant code looks
the same:

       ;; Find a dired buffer.
       (dolist (buffer (buffer-list))
	(with-current-buffer buffer
	  (when (and (derived-mode-p 'dired-mode)
	             (equal (file-truename dir)
		            (file-truename default-directory)))

That call to (file-truename default-directory) for each dired
buffer can cause tramp to spin up for closed connections for
entirely irrelevant dired buffers.

Can we put some guards in there to decide whether or not dir
and default-directory are actually on the same host before
comparing them via file-truename?


-Phil (who is simply testing (not (file-remote-p default-directory))
        as an interim fix)





In GNU Emacs 27.1 (build 2, x86_64-pc-linux-gnu, X toolkit, Xaw scroll 
bars)
  of 2020-08-20 built on mts-devtools
Windowing system distributor 'The X.Org Foundation', version 
11.0.12008000
System Description: Ubuntu 18.04.3 LTS

Recent messages:
Copying /tmp/tramp.XFX7Dz.elcNyCbqo to 
/sudo:root@mts-devtools:/usr/local/share/emacs/27.1/lisp/image-mode.elcNyCbqo...done
Renaming 
/sudo:root@mts-devtools:/usr/local/share/emacs/27.1/lisp/image-mode.elcNyCbqo 
to 
/sudo:root@mts-devtools:/usr/local/share/emacs/27.1/lisp/image-mode.elc...done
Wrote 
/sudo:root@mts-devtools:/usr/local/share/emacs/27.1/lisp/image-mode.elc
Wrote 
/sudo:root@mts-devtools:/usr/local/share/emacs/27.1/lisp/image-mode.{el,elc}
Making completion list...
Loading 
/sudo:root@mts-devtools:/usr/local/share/emacs/27.1/lisp/image-mode.elc...
Copying 
/sudo:root@mts-devtools:/usr/local/share/emacs/27.1/lisp/image-mode.elc 
to /tmp/tramp.vr7HOL.elc...done
Loading 
/sudo:root@mts-devtools:/usr/local/share/emacs/27.1/lisp/image-mode.elc...done
Hunk already applied
Mark set [2 times]

Configured using:
  'configure --with-x-toolkit=lucid --without-sound'

Configured features:
XPM JPEG TIFF GIF PNG RSVG DBUS GSETTINGS GLIB NOTIFY INOTIFY GNUTLS
LIBXML2 FREETYPE HARFBUZZ XFT ZLIB TOOLKIT_SCROLL_BARS LUCID X11 XDBE
XIM MODULES THREADS PDUMPER LCMS2 GMP

Important settings:
   value of $LC_MONETARY: en_NZ.UTF-8
   value of $LC_NUMERIC: en_NZ.UTF-8
   value of $LC_TIME: en_NZ.UTF-8
   value of $LANG: en_NZ.UTF-8
   locale-coding-system: utf-8

Major mode: Emacs-Lisp

Minor modes in effect:
   hi-lock-mode: t
   global-magit-file-mode: t
   goto-address-prog-mode: t
   elisp-slime-nav-mode: t
   hl-sexp-mode: t
   lexbind-mode: t
   idle-highlight-mode: t
   minibuffer-line-mode: t
   global-edit-server-edit-mode: t
   savehist-mode: t
   magit-wip-before-change-mode: t
   magit-wip-after-apply-mode: t
   magit-wip-after-save-mode: t
   global-git-commit-mode: t
   magit-auto-revert-mode: t
   shell-dirtrack-mode: t
   async-bytecomp-package-mode: t
   my-contextual-help-mode: t
   global-so-long-mode: t
   show-paren-mode: t
   my-visible-bell-mode: t
   cua-mode: t
   global-display-fill-column-indicator-mode: t
   display-fill-column-indicator-mode: t
   minibuffer-depth-indicate-mode: t
   which-key-mode: t
   winner-mode: t
   global-subword-mode: t
   subword-mode: t
   global-hl-line-mode: t
   hl-line-mode: t
   display-time-mode: t
   keep-buffers-mode: t
   fic-mode: t
   my-keys-local-minor-mode: t
   auto-compile-on-load-mode: t
   auto-compile-on-save-mode: t
   auto-compile-mode: t
   url-handler-mode: t
   tooltip-mode: t
   global-eldoc-mode: t
   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
   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/vagrant/.emacs.d/el-get/scratch/el-get hides 
/home/vagrant/.emacs.d/el-get/el-get/el-get
/home/vagrant/.emacs.d/my-lisp/so-long hides 
/usr/local/share/emacs/27.1/lisp/so-long
/home/vagrant/.emacs.d/lisp/python hides 
/usr/local/share/emacs/27.1/lisp/progmodes/python

Features:
(shadow emacsbug sendmail tramp-adb tramp-archive tramp-ftp tramp-gvfs
edebug image-file dabbrev image-dimensions-minor-mode image-mode exif
mhtml-mode debug backtrace magit-ediff calc-alg calc-ext calc-menu calc
calc-loaddefs calc-macs find-file diff-hl-flydiff diff-hl vc-dir ewoc
flyspell sockit web-mode rfc2104 tramp-cmds wdired nroff-mode sort align
tramp-sh repeat ispell css-mode sgml-mode git-rebase js hippie-exp
log-view ediff ediff-merg ediff-mult ediff-wind ediff-diff ediff-help
ediff-init ediff-util magit-submodule cua-rect vc-annotate vc
vc-dispatcher markdown-mode mail-extr magit-extras shr-color color
url-file url-dired adaptive-wrap eww mm-url gnus nnheader url-queue shr
svg dom php-local-manual bug-reference gitconfig-mode conf-mode
compare-w mm-archive gnutls network-stream url-cache url-dav wgrep
find-dired ido find-file-in-project etags-select sh-script ruby-mode
smie face-remap two-column iso-transl php-extras-eldoc-functions
sql-upcase sql view php-eldoc my-drupal grep scroll-lock drush-php psysh
php-mode mode-local cc-langs php-face php php-project flymake-proc
flymake cc-mode cc-fonts cc-guess cc-menus cc-cmds cc-styles cc-align
cc-engine cc-vars cc-defs docker-tramp tramp-cache tramp tramp-loaddefs
trampver tramp-integration files-x tramp-compat parse-time iso8601
ls-lisp pp cl-print mule-util imenu man term disp-table ehelp warnings
winnow compile eieio-opt speedbar sb-image ezimage dframe loadhist
ibuf-ext ibuffer ibuffer-loaddefs help-fns hi-lock misearch
multi-isearch executable finder finder-inf find-func skeleton dired-aux
magit-obsolete magit-blame magit-stash magit-bisect magit-remote
magit-commit magit-sequence magit-notes magit-worktree magit-tag
magit-merge magit-branch magit-reset magit-collab ghub url-http url-gw
nsm url-auth url url-proxy url-expand url-methods url-history url-cookie
url-domsuf url-util mailcap let-alist magit-files magit-refs
magit-status magit magit-repos magit-apply autoinsert goto-addr
project-local-variables elisp-slime-nav etags fileloop generator xref
project hl-sexp lexbind-mode idle-highlight-mode rx tabify
minibuffer-line edit-server my-org my-projects my-session savehist
desktop frameset my-theme zenburn-theme my-mail autoloads my-libraries
sudo my-version-control magit-wip magit-log magit-diff smerge-mode diff
git-commit log-edit message rmc puny rfc822 mml mml-sec epa epg
epg-config gnus-util rmail rmail-loaddefs text-property-search time-date
mm-decode mm-bodies mm-encode mail-parse rfc2231 rfc2047 rfc2045 mm-util
ietf-drums mail-prsvr mailabbrev mail-utils gmm-utils mailheader
pcvs-util add-log magit-core magit-autorevert autorevert filenotify
magit-process with-editor shell pcomplete comint server ansi-color
magit-margin magit-mode magit-git magit-section magit-utils crm
magit-popup pcase async-bytecomp async format-spec dash my-text
my-programming my-python so-long my-whitespace ws-trim my-rectangles
my-utilities browse-kill-ring my-configuration cus-edit cus-start
cus-load wid-edit dired-details dired-x highlight-parentheses paren
url-privacy delight delsel cua-base ffap display-fill-column-indicator
mb-depth which-key framemove windmove winner ring cap-words superword
subword hl-line time my-externals .loaddefs windcycle transpose-frame
simple-wiki derived sdcv-mode noutline outline sauron rainbow-mode
notify dbus xml multiple-cursors mc-separate-operations
rectangular-region-mode mc-mark-pop mc-mark-more thingatpt
mc-cycle-cursors mc-edit-lines multiple-cursors-core rect mo-git-blame
keep-buffers iedit fic-mode dtrt-indent browse-at-remote vc-git
diff-mode easy-mmode s el-get cl-extra help-mode autoload radix-tree
lisp-mnt cl dired dired-loaddefs my-elpa my-local my-keybindings edmacro
kmacro jka-compr auto-compile packed etags-select-autoloads info
project-local-variables-autoloads advice wtf-autoloads package easymenu
browse-url url-handlers url-parse auth-source cl-seq eieio eieio-core
cl-macs eieio-loaddefs password-cache json subr-x map url-vars seq
byte-opt gv bytecomp byte-compile cconv cl-loaddefs cl-lib 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 tab-bar menu-bar rfn-eshadow isearch timer
select scroll-bar mouse jit-lock font-lock syntax facemenu font-core
term/tty-colors frame minibuffer 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 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 threads dbusbind inotify lcms2 dynamic-setting
system-font-setting font-render-setting x-toolkit x multi-tty
make-network-process emacs)

Memory information:
((conses 16 6395697 774980)
  (symbols 48 99712 1)
  (strings 32 264602 49924)
  (string-bytes 1 13742442)
  (vectors 16 478511)
  (vector-slots 8 5272375 517538)
  (floats 8 2334 4710)
  (intervals 56 1397671 5095)
  (buffers 1000 1327))

bug#54606: 27.1; next/previous image command in image-mode triggers tramp connections for all remote dired buffers

[Lars Ingebrigtsen]

Phil Sainty <psainty@orcon.net.nz> writes:

>       ;; Find a dired buffer.
>       (dolist (buffer (buffer-list))
> 	(with-current-buffer buffer
> 	  (when (and (derived-mode-p 'dired-mode)
> 	             (equal (file-truename dir)
> 		            (file-truename default-directory)))
>
> That call to (file-truename default-directory) for each dired
> buffer can cause tramp to spin up for closed connections for
> entirely irrelevant dired buffers.
>
> Can we put some guards in there to decide whether or not dir
> and default-directory are actually on the same host before
> comparing them via file-truename?

Hm.  Would binding `non-essential' around that call fix the issue?  (I've
added Michael to the CCs.)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#54606: 27.1; next/previous image command in image-mode triggers tramp connections for all remote dired buffers

[Michael Albinus]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Phil Sainty <psainty@orcon.net.nz> writes:
>
>>       ;; Find a dired buffer.
>>       (dolist (buffer (buffer-list))
>> 	(with-current-buffer buffer
>> 	  (when (and (derived-mode-p 'dired-mode)
>> 	             (equal (file-truename dir)
>> 		            (file-truename default-directory)))
>>
>> That call to (file-truename default-directory) for each dired
>> buffer can cause tramp to spin up for closed connections for
>> entirely irrelevant dired buffers.
>>
>> Can we put some guards in there to decide whether or not dir
>> and default-directory are actually on the same host before
>> comparing them via file-truename?
>
> Hm.  Would binding `non-essential' around that call fix the issue?  (I've
> added Michael to the CCs.)

That's my expectation. It's on my TODO to test, but I'm occupied yet. Hope
to fix this later this week.

But of course you can beat me :-)

Best regards, Michael.

bug#54606: 27.1; next/previous image command in image-mode triggers tramp connections for all remote dired buffers

[Phil Sainty]

On 2022-03-30 02:09, Michael Albinus wrote:
> Lars Ingebrigtsen <larsi@gnus.org> writes:
>> Phil Sainty <psainty@orcon.net.nz> writes:
>>> That call to (file-truename default-directory) for each dired
>>> buffer can cause tramp to spin up for closed connections for
>>> entirely irrelevant dired buffers.
>> 
>> Hm.  Would binding `non-essential' around that call fix the issue?
> 
> That's my expectation. It's on my TODO to test, but I'm occupied yet.
> Hope to fix this later this week.

I've just tested with this, and it solves the immediate problem, but
it's slow.  In my test case it's taking ~2s to switch between images,
whereas it takes only a fraction of a second when I'm ignoring remote
dired buffers by adding (not (file-remote-p default-directory)) to
the conditions.

I have 15 remote dired buffers (about half of which are using multi-
hops), which isn't a small number but also isn't big, so I think that
`non-essential' seems inadequate here (unless this is significantly
more efficient in newer versions).


-Phil

bug#54606: 27.1; next/previous image command in image-mode triggers tramp connections for all remote dired buffers

[Michael Albinus]

[-- Attachment #1: Type: text/plain, Size: 887 bytes --]

Phil Sainty <psainty@orcon.net.nz> writes:

Hi Phil,

> I've just tested with this, and it solves the immediate problem, but
> it's slow.  In my test case it's taking ~2s to switch between images,
> whereas it takes only a fraction of a second when I'm ignoring remote
> dired buffers by adding (not (file-remote-p default-directory)) to
> the conditions.
>
> I have 15 remote dired buffers (about half of which are using multi-
> hops), which isn't a small number but also isn't big, so I think that
> `non-essential' seems inadequate here (unless this is significantly
> more efficient in newer versions).

There is also a change in Tramp of Emacs master, which is relevant in
this context. Do you have a chance to test that Emacs version?

Otherwise, does the appended patch for Emacs 27 make a difference? I
haven't tested it with Emacs 27, though.

> -Phil

Best regards, Michael.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 514 bytes --]

*** /tmp/ediffwcSDZb	2022-03-30 13:50:29.736061472 +0200
--- /home/albinus/src/emacs-27/lisp/net/tramp-sh.el	2022-03-30 13:50:18.051914192 +0200
***************
*** 4958,4963 ****
--- 4958,4964 ----
      ;; If Tramp opens the same connection within a short time frame,
      ;; there is a problem.  We shall signal this.
      (unless (or (process-live-p p)
+                 (and (processp p) (not non-essential))
  		(not (tramp-file-name-equal-p
  		      vec (car tramp-current-connection)))
  		(time-less-p

bug#54606: 27.1; next/previous image command in image-mode triggers tramp connections for all remote dired buffers

[Lars Ingebrigtsen]

Phil Sainty <psainty@orcon.net.nz> writes:

> I've just tested with this, and it solves the immediate problem, but
> it's slow.  In my test case it's taking ~2s to switch between images,
> whereas it takes only a fraction of a second when I'm ignoring remote
> dired buffers by adding (not (file-remote-p default-directory)) to
> the conditions.

Perhaps the n/p commands should just be changed.  It's really unusual to
have several dired buffers open to the same directory, and the dired
buffer we're interested in is usually the one we were in when we started
to view the image.

So perhaps image-mode should just note the dired buffer it's "called
from" (or determine that on startup), and then save that in a
buffer-local variable, and just use that buffer?  (Recomputing the
buffer if it's killed.)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#54606: 27.1; next/previous image command in image-mode triggers tramp connections for all remote dired buffers

[Michael Albinus]

[-- Attachment #1: Type: text/plain, Size: 971 bytes --]

Phil Sainty <psainty@orcon.net.nz> writes:

Hi Phil,

> In 27.1 this code is in `image-next-file'.  In master it's now
> `image-mode--directory-buffers', but the relevant code looks
> the same:
>
>       ;; Find a dired buffer.
>       (dolist (buffer (buffer-list))
> 	(with-current-buffer buffer
> 	  (when (and (derived-mode-p 'dired-mode)
> 	             (equal (file-truename dir)
> 		            (file-truename default-directory)))
>
> That call to (file-truename default-directory) for each dired
> buffer can cause tramp to spin up for closed connections for
> entirely irrelevant dired buffers.

I've reproduced the problem. As said already (?), it is similar to
bug#54542. The solution there was to wrap the code in question by
let-binding non-essential to t, and to enhance Tramp to behave
accordingly.

The Tramp change is already in the master branch, and also in the just
released Tramp 2.5.2.3 on GNU ELPA. So I've applied the following change
in master:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 635 bytes --]

diff --git a/lisp/image-mode.el b/lisp/image-mode.el
index d7dfb4336b..69af538aa7 100644
--- a/lisp/image-mode.el
+++ b/lisp/image-mode.el
@@ -1196,8 +1196,9 @@ image-mode--directory-buffers
   "Return an alist of type/buffer for all \"parent\" buffers to image FILE.
 This is normally a list of Dired buffers, but can also be archive and
 tar mode buffers."
-  (let ((buffers nil)
-        (dir (file-name-directory file)))
+  (let* ((non-essential t) ; Do not block for remote buffers.
+         (buffers nil)
+         (dir (file-name-directory file)))
     (cond
      ((and (boundp 'tar-superior-buffer)
 	   tar-superior-buffer)

[-- Attachment #3: Type: text/plain, Size: 1598 bytes --]


This changes the behavior. Recipe:

- emacs -Q /ssh:detlef:Pictures/IMG_0040.JPG
- Disable Ethernet/WLAN connection
- Type 'n' in the image buffer.

If the last step happens shortly after disabling the connection, nothing
changes, and Emacs is blocked. This is because Emacs needs some seconds
to detect the new status of the respective Tramp process (‘exited
abnormally with code 255').

However, waiting 10 seconds or longer (in my case), typing 'n'
immediately returns with the message

--8<---------------cut here---------------start------------->8---
user-error: No next file in this directory
--8<---------------cut here---------------end--------------->8---

That sounds acceptable, isn't it?

It shall also help for other dired buffers with different, already
closed, Tramp connections.
>
> Can we put some guards in there to decide whether or not dir
> and default-directory are actually on the same host before
> comparing them via file-truename?

If you want to go *this* direction, there would be a simple change:

--8<---------------cut here---------------start------------->8---
	  (when (and (derived-mode-p 'dired-mode)
	             (equal (file-remote-p dir)
		            (file-remote-p default-directory))
	             (equal (file-truename dir)
		            (file-truename default-directory)))
--8<---------------cut here---------------end--------------->8---

file-remote-p doesn't do anything on wire.

> -Phil (who is simply testing (not (file-remote-p default-directory))
>        as an interim fix)

Best regards, Michael.

bug#54606: 27.1; next/previous image command in image-mode triggers tramp connections for all remote dired buffers

[Lars Ingebrigtsen]

Michael Albinus <michael.albinus@gmx.de> writes:

>> Can we put some guards in there to decide whether or not dir
>> and default-directory are actually on the same host before
>> comparing them via file-truename?
>
> If you want to go *this* direction, there would be a simple change:
>
> 	  (when (and (derived-mode-p 'dired-mode)
> 	             (equal (file-remote-p dir)
> 		            (file-remote-p default-directory))
> 	             (equal (file-truename dir)
> 		            (file-truename default-directory)))
>
> file-remote-p doesn't do anything on wire.

I think that sounds like a good change here anyway.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#54606: 27.1; next/previous image command in image-mode triggers tramp connections for all remote dired buffers

[Phil Sainty]

On 2022-04-01 00:41, Lars Ingebrigtsen wrote:
> Michael Albinus <michael.albinus@gmx.de> writes:
> 
>>> Can we put some guards in there to decide whether or not dir
>>> and default-directory are actually on the same host before
>>> comparing them via file-truename?
>> 
>> If you want to go *this* direction, there would be a simple change:
>> 
>> 	             (equal (file-remote-p dir)
>> 		            (file-remote-p default-directory))
>> 
>> file-remote-p doesn't do anything on wire.
> 
> I think that sounds like a good change here anyway.

Agreed -- if I'd checked to see what file-remote-p actually
returned, I would done this myself from the outset.

I've tested this approach in isolation, and it works well
in my test instance.

I've not yet tested to see whether the `non-essential'
change is similarly efficient in a newer version with a
similar session.

Lars' suggestion of having these commands NOT checking
every dired buffer every time also seems very sensible.


-Phil

bug#54606: 27.1; next/previous image command in image-mode triggers tramp connections for all remote dired buffers

[Michael Albinus]

Phil Sainty <psainty@orcon.net.nz> writes:

Hi Phil,

>>> If you want to go *this* direction, there would be a simple change:
>>> 	             (equal (file-remote-p dir)
>>> 		            (file-remote-p default-directory))
>>> file-remote-p doesn't do anything on wire.
>> I think that sounds like a good change here anyway.
>
> Agreed -- if I'd checked to see what file-remote-p actually
> returned, I would done this myself from the outset.
>
> I've tested this approach in isolation, and it works well
> in my test instance.

So I've pushed this to master.

> I've not yet tested to see whether the `non-essential'
> change is similarly efficient in a newer version with a
> similar session.

It will help for buffers with a broken remote connection. I've pushed
this change as well.

> Lars' suggestion of having these commands NOT checking
> every dired buffer every time also seems very sensible.

From my pov, this bug could be closed. However, I'll keep it open in
case something else shall be changed, too. But that I won't investigate
myself.

> -Phil

Best regards, Michael.

bug#54606: 27.1; next/previous image command in image-mode triggers tramp connections for all remote dired buffers

[Lars Ingebrigtsen]

Michael Albinus <michael.albinus@gmx.de> writes:

>> Lars' suggestion of having these commands NOT checking
>> every dired buffer every time also seems very sensible.
>
>>From my pov, this bug could be closed. However, I'll keep it open in
> case something else shall be changed, too. But that I won't investigate
> myself.

I think your fix here is sufficient, so I'm closing this bug report.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no