GNU bug report logs - #62093
30.0.50; tramp: commit 8c6a463 breaks remotely staging chunks in magit

Previous Next

To reply to this bug, email your comments to 62093 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Benjamin Orthen <benjamin <at> orthen.net>
To: bug-gnu-emacs <at> gnu.org
Subject: 30.0.50; tramp: commit 8c6a463 breaks remotely staging chunks in magit
Date: Fri, 10 Mar 2023 10:42:19 +0000

Commit 8c6a4639318f09720cda295e6a93677153046d84 breaks remotely staging
chunks in magit, see https://github.com/magit/magit/issues/4720.

This commit added the options "-icanon min 1 time 0" for non-Darwin 
remote
hosts. Without this additional option, staging chunks works.

To reproduce:
- use tramp to visit a remote Linux host
- open magit on a git repository on the remote host
- in the magit buffer, try to stage a chunk (not a whole file)


In GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version
3.24.36, cairo version 1.16.0)
Repository revision: 5ff018524c740c77215ddb5d5983dbfcadb05599
Repository branch: master
System Description: Ubuntu 22.10

Configured using:
 'configure
 
--prefix=/nix/store/bsqwccil9xvz9gvnhgqyhp4vf074c1d9-emacs-pgtk-20230308.0
 --disable-build-details --with-modules --with-pgtk
 --with-native-compilation'

Configured features:
CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GSETTINGS HARFBUZZ JPEG JSON
LIBSELINUX LIBSYSTEMD LIBXML2 MODULES NATIVE_COMP NOTIFY INOTIFY PDUMPER
PGTK PNG RSVG SECCOMP SOUND SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS
TREE_SITTER WEBP XIM GTK3 ZLIB

Important settings:
  value of $EMACSLOADPATH: 
/nix/store/q5g9xjavpivmnr4j8qlbshzj4ix3sgbl-emacs-packages-deps/share/emacs/site-lisp:
  value of $EMACSNATIVELOADPATH: 
/nix/store/q5g9xjavpivmnr4j8qlbshzj4ix3sgbl-emacs-packages-deps/share/emacs/native-lisp::
  value of $LC_MONETARY: de_DE.UTF-8
  value of $LC_NUMERIC: de_DE.UTF-8
  value of $LC_TIME: de_DE.UTF-8
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: @im=ibus
  locale-coding-system: utf-8

Message #8 received at 62093 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Benjamin Orthen <benjamin <at> orthen.net>
Cc: 62093 <at> debbugs.gnu.org
Subject: Re: bug#62093: 30.0.50; tramp: commit 8c6a463 breaks remotely
 staging chunks in magit
Date: Sat, 25 Mar 2023 18:37:04 +0100

Benjamin Orthen <benjamin <at> orthen.net> writes:

Hi Benjamin,

> To reproduce:
> - use tramp to visit a remote Linux host
> - open magit on a git repository on the remote host
> - in the magit buffer, try to stage a chunk (not a whole file)

I'm sorry, but I don't use magit myself. Could you please describe the
magit commands I have to apply in Emacs in order to reproduce?

Best regards, Michael.

Message #11 received at 62093 <at> debbugs.gnu.org (full text, mbox):

From: Benjamin Orthen <benjamin <at> orthen.net>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: 62093 <at> debbugs.gnu.org
Subject: Re: bug#62093: 30.0.50; tramp: commit 8c6a463 breaks remotely staging
 chunks in magit
Date: Tue, 04 Apr 2023 09:13:06 +0000

[Message part 1 (text/plain, inline)]

Hi Michael,

sure. You'll need a git repository on a remote host with unstaged 
changes.

In the git repository, execute:
- `magit-status`
- `magit-jump-to-unstaged`
- navigate the cursor to one unstaged chunk in the diff (see attached 
image as an example)
- `magit-stage`

I hope this helps for reproducing the issue.

Best,
Benjamin


Am 2023-03-25 17:37, schrieb Michael Albinus:
> Benjamin Orthen <benjamin <at> orthen.net> writes:
> 
> Hi Benjamin,
> 
>> To reproduce:
>> - use tramp to visit a remote Linux host
>> - open magit on a git repository on the remote host
>> - in the magit buffer, try to stage a chunk (not a whole file)
> 
> I'm sorry, but I don't use magit myself. Could you please describe the
> magit commands I have to apply in Emacs in order to reproduce?
> 
> Best regards, Michael.

[Screenshot from 2023-04-04 11-08-54.png (image/png, attachment)]

Message #14 received at 62093 <at> debbugs.gnu.org (full text, mbox):

From: Marc Bowes <marcbowes <at> gmail.com>
To: 62093 <at> debbugs.gnu.org
Subject: Re: bug#62093: 30.0.50; tramp: commit 8c6a463 breaks remotely
Date: Thu, 1 Jun 2023 21:18:36 -0700

[Message part 1 (text/plain, inline)]

I'm having the same experience as described. I downgraded this function
(see the instructions in the magit issue), and that fixed the problem for
me.

For what it's worth, on the remote host I see hung `git` processes for each
time I tried this and bailed with C-g. If I strace any one, it's stuck
reading stdin. (To see this, you need to use ControlMaster, otherwise the
process is reaped by the ssh parent ending.)

I think the hang is caused by magit sending a patch, followed by EOF and
then the process never ends, so magit waits forever, and the patch is never
applied.

[Message part 2 (text/html, inline)]

Message #17 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Aleksander Trofimowicz <trof <at> n90.eu>
To: bug-gnu-emacs <at> gnu.org
Cc: tramp-devel <at> gnu.org
Subject: bug#62093: [PATCH] Let processes read nothing from stdin in tramp
Date: Wed, 25 Oct 2023 11:40:13 +0000

[Message part 1 (text/plain, inline)]

This minor patch is about adjustments in the terminal line settings.

There are programs, which control flow depends on receiving 0
from a read call on stdin. A notable example is git.

--
Thanks,
at

[0001-Let-processes-read-nothing-from-stdin-in-tramp.patch (text/x-patch, inline)]

From c4397c3261b9188262a1adee278075893410fb60 Mon Sep 17 00:00:00 2001
From: Aleksander Trofimowicz <trof <at> n90.eu>
Date: Wed, 25 Oct 2023 11:02:00 +0000
Subject: [PATCH] Let processes read nothing from stdin in tramp

There are programs, which control flow depends on receiving 0
from a read call on stdin. A notable example is git.

* lisp/net/tramp-sh.el (tramp-sh-handle-make-process): Use read
timeout instead of a minimal amount of data to be read in the
terminal line settings. (Bug#62093)
---
 lisp/net/tramp-sh.el | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lisp/net/tramp-sh.el b/lisp/net/tramp-sh.el
index ba6dbdf0c39..a26c1e3fcc0 100644
--- a/lisp/net/tramp-sh.el
+++ b/lisp/net/tramp-sh.el
@@ -3093,9 +3093,9 @@ tramp-sh-handle-make-process
 			      ;; FIXME: Shall we rather use "stty raw"?
 			      (if (tramp-check-remote-uname v "Darwin")
 				  (tramp-send-command
-				   v "stty -icanon min 1 time 0")
+				   v "stty -icanon min 0 time 1")
 				(tramp-send-command
-				 v "stty -icrnl -icanon min 1 time 0")))
+				 v "stty -icrnl -icanon min 0 time 1")))
 			    ;; `tramp-maybe-open-connection' and
 			    ;; `tramp-send-command-and-read' could
 			    ;; have trashed the connection buffer.
-- 
2.42.0

Message #20 received at 62093 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Aleksander Trofimowicz <trof <at> n90.eu>
Cc: Benjamin Orthen <benjamin <at> orthen.net>, 62093 <at> debbugs.gnu.org
Subject: Re: bug#62093: [PATCH] Let processes read nothing from stdin in tramp
Date: Sat, 04 Nov 2023 18:42:06 +0100

Aleksander Trofimowicz <trof <at> n90.eu> writes:

Hi Alexander,

> This minor patch is about adjustments in the terminal line settings.
>
> There are programs, which control flow depends on receiving 0
> from a read call on stdin. A notable example is git.

I wanted to let you know that I found time to check this. Thanks for the
patch.

I can confirm, that I could reproduce the problem with the recipe given
by Benjamin. And your patch fixes this in my local environment.

Since it is a very low level change, I ran a full test campaign with
recent Tramp 2.6 and your patch over the last days. I've used
tramp-tests.el for this, targeting many different remote hosts. 60
different configurations have passed, but unfortunately, 3
configurations have failed. All of them did fail like

--8<---------------cut here---------------start------------->8---
Running 92 tests (2023-11-04 07:23:29+0100, selector ‘(not (tag :unstable))’)
Remote directory: ‘/sshx:ford|su:albinus@|ssh:albinus <at> detlef|sudo::/root/tmp’
...
Test tramp-test30-make-process condition:
    (ert-test-failed "`tramp-test30-make-process' timed out")
   FAILED  56/92  tramp-test30-make-process (31.354531 sec) at tramp-tests.el:5354
--8<---------------cut here---------------end--------------->8---

The failed test runs have in common that there is a complex Tramp target
for the test using multi-hops, like above. I suspect a problem with
timing due to the several hops, but I don't know exactly what's up.

Will continue to debug. Just to let you know.

> Thanks,
> at

Best regards, Michael.

Message #23 received at 62093 <at> debbugs.gnu.org (full text, mbox):

From: Aleksander Trofimowicz <trof <at> n90.eu>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: Benjamin Orthen <benjamin <at> orthen.net>, 62093 <at> debbugs.gnu.org
Subject: Re: bug#62093: [PATCH] Let processes read nothing from stdin in tramp
Date: Mon, 06 Nov 2023 16:51:24 +0000

Hi Michael,

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

> I wanted to let you know that I found time to check this. Thanks for the
> patch.
>
Thank you for looking into this issue.

> The failed test runs have in common that there is a complex Tramp target
> for the test using multi-hops, like above. I suspect a problem with
> timing due to the several hops, but I don't know exactly what's up.
>
Triggered by your multi-hop tests (host B via host A) I run git-apply
actions over Tramp with the patch I submitted. I settled with 3
different test environment configurations; all of them should provide
the same functionality:

1. An explicit multi-hop target "/ssh:host_A|/ssh:host_B:..."

2. The SSH client option ProxyCommand set to "ssh host_A nc host_B 22" and
simple "/ssh:host_B"

3. The SSH client option ProxyJump set to "host_A" and simple "/ssh:host_B"

tramp-use-connection-share was set to nil in each case.

It worked as expected only in the first two cases. As far as the last one
is concerned, the workflow wasn't stalled, but it turned out to be no-op
after all.

It seems the last test environment enables the most responsive data
stream: no additional userland process is forked on a middle box and
Nagle's algorithm is disabled for all TCP connections involved (which is
not true for the case no 2.). In the end such results might corroborate
your theory.

--
at

Message #26 received at 62093 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Aleksander Trofimowicz <trof <at> n90.eu>
Cc: Benjamin Orthen <benjamin <at> orthen.net>, 62093 <at> debbugs.gnu.org
Subject: Re: bug#62093: [PATCH] Let processes read nothing from stdin in tramp
Date: Sat, 11 Nov 2023 14:08:18 +0100

[Message part 1 (text/plain, inline)]

Aleksander Trofimowicz <trof <at> n90.eu> writes:

> Hi Michael,

Hi Alexander,

>> The failed test runs have in common that there is a complex Tramp target
>> for the test using multi-hops, like above. I suspect a problem with
>> timing due to the several hops, but I don't know exactly what's up.
>>
> Triggered by your multi-hop tests (host B via host A) I run git-apply
> actions over Tramp with the patch I submitted. I settled with 3
> different test environment configurations; all of them should provide
> the same functionality:
>
> 1. An explicit multi-hop target "/ssh:host_A|/ssh:host_B:..."
>
> 2. The SSH client option ProxyCommand set to "ssh host_A nc host_B 22" and
> simple "/ssh:host_B"
>
> 3. The SSH client option ProxyJump set to "host_A" and simple "/ssh:host_B"
>
> tramp-use-connection-share was set to nil in each case.
>
> It worked as expected only in the first two cases. As far as the last one
> is concerned, the workflow wasn't stalled, but it turned out to be no-op
> after all.
>
> It seems the last test environment enables the most responsive data
> stream: no additional userland process is forked on a middle box and
> Nagle's algorithm is disabled for all TCP connections involved (which is
> not true for the case no 2.). In the end such results might corroborate
> your theory.

Well, I've tried deeper debugging with this. But for whatever values
I've taken for stty's min and time in tramp-sh-handle-make-process,
there were always cases it didn't work. Rare cases, and not always
reproducible due to race conditions, but they exist.

Going a step back to magit, I see the following comment in magit-start-process:

--8<---------------cut here---------------start------------->8---
               ;; Don't use a pty, because it would set icrnl
               ;; which would modify the input (issue #20).
--8<---------------cut here---------------end--------------->8---

I cannot speak for the local case. But in Tramp, we need to set "stty
-icanon ..." for the pipe connection type in order avoid blocking
situations with larger hunks of data, see the comment there. And the use
case of magit-start-process would be better with the connection type
pty, at least when calling Tramp.

The appended (rather naïve) patch fixes the reported use case in my
local enviroment.

> at

Best regards, Michael.

[Message part 2 (text/x-patch, attachment)]

Message #29 received at 62093 <at> debbugs.gnu.org (full text, mbox):

From: Aleksander Trofimowicz <trof <at> n90.eu>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: Benjamin Orthen <benjamin <at> orthen.net>, 62093 <at> debbugs.gnu.org
Subject: Re: bug#62093: [PATCH] Let processes read nothing from stdin in tramp
Date: Fri, 17 Nov 2023 20:10:34 +0000

Hi Michael,

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

> I cannot speak for the local case. But in Tramp, we need to set "stty
> -icanon ..." for the pipe connection type in order avoid blocking
> situations with larger hunks of data, see the comment there. And the use
> case of magit-start-process would be better with the connection type
> pty, at least when calling Tramp.
>
Re-configuring a TTY device via "-icanon" does not only lift the 4k input
line limit; there are other far-reaching consequences. Most notably: a
handful of the terminal special characters are delivered straight to
stdin of a managed process, which more often than not is unprepared to
handle such a situation (e.g. the VEOF character). I would argue in the
Tramp context doubly so, since the connection-type is set to 'pipe'
somewhere up in a stack. It also invalidates certain expectations most
people have while invoking process-send-eof.

These are reasons magit stopped working, and my patch was a half-baked
attempt to address the problem by other means. Please drop it. At the
same time I would encourage you to revert the change that introduced the
noncanonical mode, since the current state is very confusing and breaks
existing code. A dedicated defcustom should cater for those who deal
with large input data.

Ideally no pty should be allocated if one asks for 'pipe' (by employing
ssh connection multiplexing?).
--
Kind regards,
at

Message #32 received at 62093 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Aleksander Trofimowicz <trof <at> n90.eu>
Cc: Benjamin Orthen <benjamin <at> orthen.net>, 62093 <at> debbugs.gnu.org
Subject: Re: bug#62093: [PATCH] Let processes read nothing from stdin in tramp
Date: Wed, 29 Nov 2023 14:27:04 +0100

[Message part 1 (text/plain, inline)]

Aleksander Trofimowicz <trof <at> n90.eu> writes:

> Hi Michael,

Hi Aleksander,

> These are reasons magit stopped working, and my patch was a half-baked
> attempt to address the problem by other means. Please drop it. At the
> same time I would encourage you to revert the change that introduced the
> noncanonical mode, since the current state is very confusing and breaks
> existing code. A dedicated defcustom should cater for those who deal
> with large input data.

I've tried several approaches, none of them did satisfy me
completely. So I have introduced a new user option
tramp-pipe-stty-settings, which you can use for different stty
settings. See appended patch. Let-binding this to "-icanon min 0 time 1"
in magit-start-process has at least solved the problem as shown here in
this bug report. I'm not very happy with this, but it is the best I
could do now. Could you please check?

> Ideally no pty should be allocated if one asks for 'pipe' (by employing
> ssh connection multiplexing?).

We have already ssh multiplexing. If you like to avoid pty's at all, you
might try to use direct async processes:

(info "(tramp)Improving performance of asynchronous remote processes")

> Kind regards,
> at

Best regards, Michael.

[Message part 2 (text/x-patch, attachment)]

Message #35 received at 62093 <at> debbugs.gnu.org (full text, mbox):

From: Aleksander Trofimowicz <trof <at> n90.eu>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: Benjamin Orthen <benjamin <at> orthen.net>, 62093 <at> debbugs.gnu.org
Subject: Re: bug#62093: [PATCH] Let processes read nothing from stdin in tramp
Date: Mon, 04 Dec 2023 11:05:56 +0000

Hi Michael,

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

> I've tried several approaches, none of them did satisfy me
> completely. So I have introduced a new user option
> tramp-pipe-stty-settings, which you can use for different stty
> settings. See appended patch. Let-binding this to "-icanon min 0 time 1"
> in magit-start-process has at least solved the problem as shown here in
> this bug report. I'm not very happy with this, but it is the best I
> could do now. Could you please check?
>
The patch works as expected. However given the default value of
tramp-pipe-stty-settings you opted for, as well as the contents of
additional commentary in the code provided by this patch, I have got an
irresistible feeling I failed to convey the consequences of maintaining
the status quo. So let me show two simple cases:

1) open a terminal window on your local host

1.1. confirm it works in the canonical mode

$ stty -a|grep icanon

1.2. write 4 "E" characters to a random file, type ^D once, then write 4
more "E"s, and finally type ^D twice:

$ cat > /tmp/testfile
EEEEEEEE

1.3. check the contents of the target file with a hex dump
utility. Assuming you don't use fancy locale settings, this should look
like the following:

$ xxd /tmp/testfile
00000000: 4545 4545 4545 4545                      EEEEEEEE

2) now the same workflow but in the non-canonical mode

2.1. disable the canonical mode:

$ stty -icanon; stty -a|grep icanon

2. write 4 "E" characters to a random file, type ^D once, then write 4
more "E"s, and finally type ^D twice, and observe that the special
characters are echoed back and a cat process refuses to terminate. Type
^C to actually terminate the process:

$ cat > /tmp/testfile2
EEEE^DEEEE^D^D^C

2.3 check the contents of the file

$ xxd /tmp/testfile2
00000000: 4545 4545 0445 4545 4504 04              EEEE.EEEE..

As you can see there are three additional bytes (carrying 0x04 values),
each one represents the VEOF character which in the canonical mode is
intercepted by the kernel TTY subsystem, and alters the way kernel
interacts with a user process. In the non-canonical mode it is simply
passed to userspace.

Indeed very few programs are prepared to handle those terminal special
characters, and if you use one that doesn't then at best it hangs. In
less favourable circumstances the process might silently corrupt data
along the way or produce other unpleasant results.

In the Magit case it is not Magit/Emacs itself that hangs, but a git
process upon a read syscall on pts/0 on a remote host:

$ ps axjf # filtered for brevity

   PPID     PID    PGID     SID TTY        TPGID STAT   UID   TIME COMMAND
      1     977     977     977 ?             -1 Ss       0   0:00 sshd: /usr/bin/sshd -D [listener] 0 of 10-100 startups
    977  251082  251082  251082 ?             -1 Ss       0   0:00  \_ sshd: guest [priv]
 251082  251087  251082  251082 ?             -1 S     1000   0:00  |   \_ sshd: guest <at> pts/0
 251087  251089  251089  251089 pts/0     251089 Ss+   1000   0:00  |       \_ git --no-pager --literal-pathspecs -c core.preloadindex=true -c log.showSignature=false -c color.ui=false -c color.diff=false apply --cached -p0 --ignore-space-change -

$ cat /proc/251089/status|head -n7
Name:	git
Umask:	0022
State:	S (sleeping)
Tgid:	251089
Ngid:	0
Pid:	251089
PPid:	251087
[...]

# cat /proc/251089/stack
[<0>] wait_woken+0x54/0x60
[<0>] n_tty_read+0x588/0x640
[<0>] tty_read+0x134/0x250
[<0>] vfs_read+0x1fe/0x350
[<0>] ksys_read+0x6f/0xf0
[<0>] do_syscall_64+0x5d/0x90
[<0>] entry_SYSCALL_64_after_hwframe+0x6e/0xd8

The git program in the apply mode expects to receive 0 upon read() in
order conclude its processing. If tramp-pipe-stty-settings is set to
'-icanon min 1 time 0', that is never going to happen, since Magit
resorts to use process-send-eof to designate the end of input data,
which in turn sends VEOF if it is detected that pty is in use (and it
is, since this is what Tramp does behind the scene). The VEOF character
looses its special meaning in the non-canonical mode hence a deadlock
ensues.

Given above I would argue the canonical mode should be set as the
default one.

>
> We have already ssh multiplexing. If you like to avoid pty's at all, you
> might try to use direct async processes:
>
> (info "(tramp)Improving performance of asynchronous remote processes")
>
In my case that's the optimal solution. Thank you!

--
Kind regards,
at

Message #38 received at 62093 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Aleksander Trofimowicz <trof <at> n90.eu>
Cc: Benjamin Orthen <benjamin <at> orthen.net>, 62093 <at> debbugs.gnu.org
Subject: Re: bug#62093: [PATCH] Let processes read nothing from stdin in tramp
Date: Sun, 24 Dec 2023 11:32:37 +0100

Aleksander Trofimowicz <trof <at> n90.eu> writes:

> Hi Michael,

Hi Aleksander,

> Given above I would argue the canonical mode should be set as the
> default one.

Canonical mode was used in the past, and there were other use cases
which have shown problems. There won't be a one-fits-all solution.

That's why I have pushed the proposed change (new defcustom
tramp-pipe-stty-settings), which could help magit and other packages to
influence the settings. Will appear on GNU ELPA as Tramp 2.6.2 in a
couple of days.

>> We have already ssh multiplexing. If you like to avoid pty's at all, you
>> might try to use direct async processes:
>>
>> (info "(tramp)Improving performance of asynchronous remote processes")
>>
> In my case that's the optimal solution. Thank you!

Godd to know.

> Kind regards,
> at

Best regards, Michael.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.