esh-proc test failures

esh-proc test failures

[Lars Ingebrigtsen]

Some esh-proc tests seem to be failing here:

https://hydra.nixos.org/build/187349852/nixlog/1

And I've seen the same myself, but intermittently, so it seems like some
of the (new) tests there aren't quite stable.

Re: esh-proc test failures

[Jim Porter]

On 8/14/2022 11:06 AM, Lars Ingebrigtsen wrote:
> Some esh-proc tests seem to be failing here:
> 
> https://hydra.nixos.org/build/187349852/nixlog/1
> 
> And I've seen the same myself, but intermittently, so it seems like some
> of the (new) tests there aren't quite stable.

 From Hydra, it seems that the issue is that there's a broken pipe, so 
there's probably a timing issue with how the external process runs vs 
Eshell's internal code. I'll try to reproduce this locally and fix it.

Re: esh-proc test failures

[Jim Porter]

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

On 8/14/2022 11:44 AM, Jim Porter wrote:
> On 8/14/2022 11:06 AM, Lars Ingebrigtsen wrote:
>> Some esh-proc tests seem to be failing here:
>>
>> https://hydra.nixos.org/build/187349852/nixlog/1
>>
>> And I've seen the same myself, but intermittently, so it seems like some
>> of the (new) tests there aren't quite stable.
> 
>  From Hydra, it seems that the issue is that there's a broken pipe, so 
> there's probably a timing issue with how the external process runs vs 
> Eshell's internal code. I'll try to reproduce this locally and fix it.

Ok, after stepping away for a bit and letting this bubble around in my 
head, the issue turned out to be pretty simple (I think). Attached is a 
patch that should fix this.

Here's what's going on. Imagine you have a pipeline like 'lisp-command | 
external-command'. It could operate in this order:

   * Start external-command
   * Send output of lisp-command to external-command
   * Finish external-command

That's good. However, it could also do this:

   * Start external-command
   * Finish external-command
   * Send output of lisp-command to external-command -> SIGPIPE!

The fix is to make sure that lisp commands also handle SIGPIPE (aka 
'eshell-broken-pipe'), just like how external commands do.

This is actually a bug introduced from the patch for bug#54062, but 
luckily the tests I added for bug#56025 revealed the issue.

[-- Attachment #2: 0001-Handle-eshell-pipe-broken-when-evaluating-Lisp-forms.patch --]
[-- Type: text/plain, Size: 2147 bytes --]

From 7360ecfc73781c83dc9f6c55c7cb7366e5472895 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Mon, 22 Aug 2022 09:53:24 -0700
Subject: [PATCH] Handle 'eshell-pipe-broken' when evaluating Lisp forms in
 Eshell

* lisp/eshell/esh-cmd.el (eshell-exec-lisp): Handle
'eshell-pipe-broken'.

* test/lisp/eshell/esh-proc-tests.el
(esh-proc-test/pipeline-connection-type/middle)
(esh-proc-test/pipeline-connection-type/last): Remove ':unstable'.
---
 lisp/eshell/esh-cmd.el             | 4 ++++
 test/lisp/eshell/esh-proc-tests.el | 4 ----
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 2f77f3f497..d5cc3706fd 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -1347,6 +1347,10 @@ eshell-exec-lisp
                  (apply func-or-form args)))))
         (and result (funcall printer result))
         result)
+    (eshell-pipe-broken
+     ;; 141 is 128 + 13 (the numeric value of SIGPIPE).
+     (setq eshell-last-command-status 141)
+     nil)
     (error
      (setq eshell-last-command-status 1)
      (let ((msg (error-message-string err)))
diff --git a/test/lisp/eshell/esh-proc-tests.el b/test/lisp/eshell/esh-proc-tests.el
index 62e784e8f6..2369bb5cc0 100644
--- a/test/lisp/eshell/esh-proc-tests.el
+++ b/test/lisp/eshell/esh-proc-tests.el
@@ -74,8 +74,6 @@ esh-proc-test/pipeline-connection-type/first
 (ert-deftest esh-proc-test/pipeline-connection-type/middle ()
   "Test that all streams are pipes when a command is in the middle of a
 pipeline."
-  ;; Repeated unreproducible errors.
-  :tags '(:unstable)
   (skip-unless (and (executable-find "sh")
                     (executable-find "cat")))
   (eshell-command-result-equal
@@ -84,8 +82,6 @@ esh-proc-test/pipeline-connection-type/middle
 
 (ert-deftest esh-proc-test/pipeline-connection-type/last ()
   "Test that only output streams are PTYs when a command ends a pipeline."
-  ;; Repeated unreproducible errors.
-  :tags '(:unstable)
   (skip-unless (executable-find "sh"))
   (eshell-command-result-equal
    (concat "echo | " esh-proc-test--detect-pty-cmd)
-- 
2.25.1

Re: esh-proc test failures

[Eli Zaretskii]

> From: Jim Porter <jporterbugs@gmail.com>
> Cc: emacs-devel@gnu.org
> Date: Mon, 22 Aug 2022 10:06:36 -0700
> 
> diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
> index 2f77f3f497..d5cc3706fd 100644
> --- a/lisp/eshell/esh-cmd.el
> +++ b/lisp/eshell/esh-cmd.el
> @@ -1347,6 +1347,10 @@ eshell-exec-lisp
>                   (apply func-or-form args)))))
>          (and result (funcall printer result))
>          result)
> +    (eshell-pipe-broken
> +     ;; 141 is 128 + 13 (the numeric value of SIGPIPE).
> +     (setq eshell-last-command-status 141)
> +     nil)

This is non-portable, I think on two counts:

 . the assumption that the exit code is the signal number left-shifted
   by N bits (btw, isn't N = 8, not 7?)
 . the assumption that SIGPIPE is signal 13 (does Posix mandate that?)

What do we expect to happen here on MS-Windows and other non-Posix
platforms, where both of the above assumptions are false?

Re: esh-proc test failures

[Jim Porter]

On 8/22/2022 11:56 AM, Eli Zaretskii wrote:
>> From: Jim Porter <jporterbugs@gmail.com>
>> Cc: emacs-devel@gnu.org
>> Date: Mon, 22 Aug 2022 10:06:36 -0700
>>
>> diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
>> index 2f77f3f497..d5cc3706fd 100644
>> --- a/lisp/eshell/esh-cmd.el
>> +++ b/lisp/eshell/esh-cmd.el
>> @@ -1347,6 +1347,10 @@ eshell-exec-lisp
>>                    (apply func-or-form args)))))
>>           (and result (funcall printer result))
>>           result)
>> +    (eshell-pipe-broken
>> +     ;; 141 is 128 + 13 (the numeric value of SIGPIPE).
>> +     (setq eshell-last-command-status 141)
>> +     nil)
> 
> This is non-portable, I think on two counts:
> 
>   . the assumption that the exit code is the signal number left-shifted
>     by N bits (btw, isn't N = 8, not 7?)
>   . the assumption that SIGPIPE is signal 13 (does Posix mandate that?)
> 
> What do we expect to happen here on MS-Windows and other non-Posix
> platforms, where both of the above assumptions are false?

The only thing that really needs to happen here is that the signal is 
caught so it doesn't bubble up past this point and break things. The 
command status could be anything really, and I'm pretty sure Eshell 
doesn't even allow inspecting this value (yet), since it would only 
occur for a non-last item in a pipeline. (In the future, Eshell could 
offer something like $PIPESTATUS to examine this.)

I could expand the comment to explain that this value is 
somewhat-arbitrary and just designed to match GNU/Linux. Alternately, if 
there's a way to inspect the system's conventions to use here (e.g. 
getting the numeric value of SIGPIPE for the current system), we could 
do that too.

Re: esh-proc test failures

[Eli Zaretskii]

> Cc: larsi@gnus.org, emacs-devel@gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> Date: Mon, 22 Aug 2022 12:23:37 -0700
> 
> >> +    (eshell-pipe-broken
> >> +     ;; 141 is 128 + 13 (the numeric value of SIGPIPE).
> >> +     (setq eshell-last-command-status 141)
> >> +     nil)
> > 
> > This is non-portable, I think on two counts:
> > 
> >   . the assumption that the exit code is the signal number left-shifted
> >     by N bits (btw, isn't N = 8, not 7?)
> >   . the assumption that SIGPIPE is signal 13 (does Posix mandate that?)
> > 
> > What do we expect to happen here on MS-Windows and other non-Posix
> > platforms, where both of the above assumptions are false?
> 
> The only thing that really needs to happen here is that the signal is 
> caught so it doesn't bubble up past this point and break things.

How do you accomplish that?  On MS-Windows there's no SIGPIPE signal,
for example.

> The 
> command status could be anything really, and I'm pretty sure Eshell 
> doesn't even allow inspecting this value (yet), since it would only 
> occur for a non-last item in a pipeline. (In the future, Eshell could 
> offer something like $PIPESTATUS to examine this.)

Not sure I understand completely what you are saying here, but AFAIR
writing to a closed pipe on MS-Windows will cause EINVAL errno.

> I could expand the comment to explain that this value is 
> somewhat-arbitrary and just designed to match GNU/Linux.

Yes, please.

> Alternately, if there's a way to inspect the system's conventions to
> use here (e.g.  getting the numeric value of SIGPIPE for the current
> system), we could do that too.

I might be able to help if I understand better what is needed here.

Thanks.

Re: esh-proc test failures

[Jim Porter]

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

On 8/22/2022 7:27 PM, Eli Zaretskii wrote:
>> Cc: larsi@gnus.org, emacs-devel@gnu.org
>> From: Jim Porter <jporterbugs@gmail.com>
>> Date: Mon, 22 Aug 2022 12:23:37 -0700
>>
>>>> +    (eshell-pipe-broken
>>>> +     ;; 141 is 128 + 13 (the numeric value of SIGPIPE).
>>>> +     (setq eshell-last-command-status 141)
>>>> +     nil)
>>>
>>> This is non-portable, I think on two counts:
>>>
>>>    . the assumption that the exit code is the signal number left-shifted
>>>      by N bits (btw, isn't N = 8, not 7?)
>>>    . the assumption that SIGPIPE is signal 13 (does Posix mandate that?)
>>>
>>> What do we expect to happen here on MS-Windows and other non-Posix
>>> platforms, where both of the above assumptions are false?
>>
>> The only thing that really needs to happen here is that the signal is
>> caught so it doesn't bubble up past this point and break things.
> 
> How do you accomplish that?  On MS-Windows there's no SIGPIPE signal,
> for example.

If Eshell tries to write to a process object and it fails, it gets 
treated as a broken pipe. Technically, it signals 'eshell-pipe-broken', 
but that's roughly equivalent to SIGPIPE. This is mainly so that in a 
pipeline like "foo | bar", if "bar" terminates, Eshell can inform "foo" 
of the fact; on POSIX systems, it would send SIGPIPE, but on MS Windows, 
it just calls 'delete-process'. This is important because we want to be 
sure that if you have a pipeline like "yes | head", the "yes" gets 
stopped once "head" is done.

Eshell's implementation isn't perfect though; since Emacs uses process 
filters, Eshell actually sends the SIGPIPE later than otherwise 
expected. Still, it's probably better than hanging indefinitely. (Fixing 
this "properly" would probably require dramatically reworking how Emacs 
interacts with subprocesses, which I think would be too risky to do, 
since we don't want to break things in process.c.)

For the actual code that raises this signal, see the end of the function 
'eshell-output-object-to-target' in lisp/eshell/esh-io.el.

>> The
>> command status could be anything really, and I'm pretty sure Eshell
>> doesn't even allow inspecting this value (yet), since it would only
>> occur for a non-last item in a pipeline. (In the future, Eshell could
>> offer something like $PIPESTATUS to examine this.)
> 
> Not sure I understand completely what you are saying here, but AFAIR
> writing to a closed pipe on MS-Windows will cause EINVAL errno.

Indeed, it would be nice if we could force things so that an MS Windows 
program gets EINVAL for its WriteFile call, but because Eshell only 
interacts indirectly with the program's output, it's too late to do that 
by the time Eshell responds.

>> I could expand the comment to explain that this value is
>> somewhat-arbitrary and just designed to match GNU/Linux.
> 
> Yes, please.

How does the attached look?

>> Alternately, if there's a way to inspect the system's conventions to
>> use here (e.g.  getting the numeric value of SIGPIPE for the current
>> system), we could do that too.
> 
> I might be able to help if I understand better what is needed here.

Well, it depends on what we think users would expect. Currently, I don't 
think Eshell provides the necessary functionality to tell when the 
process "foo" fails (i.e. returns a non-zero exit status) in the 
pipeline "foo | bar".

However, if it did, I think the most a user would care about is that 
there's some consistent way of telling what went wrong. If there were a 
way to determine what convention the current system uses to say "foo 
terminated because it tried to write to bar after bar exited", we could 
use that.

I'm not sure this part is worth spending a lot of time on though, 
especially since the exit status of "foo" isn't currently accessible as 
far as I know.

[-- Attachment #2: 0001-Handle-eshell-pipe-broken-when-evaluating-Lisp-forms.patch --]
[-- Type: text/plain, Size: 2489 bytes --]

From 69f84b4101313abc025189b2dbdc57d9b6e4849e Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Mon, 22 Aug 2022 09:53:24 -0700
Subject: [PATCH] Handle 'eshell-pipe-broken' when evaluating Lisp forms in
 Eshell

* lisp/eshell/esh-cmd.el (eshell-exec-lisp): Handle
'eshell-pipe-broken'.

* test/lisp/eshell/esh-proc-tests.el
(esh-proc-test/pipeline-connection-type/middle)
(esh-proc-test/pipeline-connection-type/last): Remove ':unstable'.
---
 lisp/eshell/esh-cmd.el             | 9 +++++++++
 test/lisp/eshell/esh-proc-tests.el | 4 ----
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 2f77f3f497..3886a1c086 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -1347,6 +1347,15 @@ eshell-exec-lisp
                  (apply func-or-form args)))))
         (and result (funcall printer result))
         result)
+    (eshell-pipe-broken
+     ;; If FUNC-OR-FORM tried and failed to write some output to a
+     ;; process, it will raise an `eshell-pipe-broken' signal (this is
+     ;; analogous to SIGPIPE on POSIX systems).  In this case, set the
+     ;; command status to some non-zero value to indicate an error; to
+     ;; match GNU/Linux, we use 141, which is 128 + 13 (the numeric
+     ;; value of SIGPIPE on GNU/Linux).
+     (setq eshell-last-command-status 141)
+     nil)
     (error
      (setq eshell-last-command-status 1)
      (let ((msg (error-message-string err)))
diff --git a/test/lisp/eshell/esh-proc-tests.el b/test/lisp/eshell/esh-proc-tests.el
index 62e784e8f6..2369bb5cc0 100644
--- a/test/lisp/eshell/esh-proc-tests.el
+++ b/test/lisp/eshell/esh-proc-tests.el
@@ -74,8 +74,6 @@ esh-proc-test/pipeline-connection-type/first
 (ert-deftest esh-proc-test/pipeline-connection-type/middle ()
   "Test that all streams are pipes when a command is in the middle of a
 pipeline."
-  ;; Repeated unreproducible errors.
-  :tags '(:unstable)
   (skip-unless (and (executable-find "sh")
                     (executable-find "cat")))
   (eshell-command-result-equal
@@ -84,8 +82,6 @@ esh-proc-test/pipeline-connection-type/middle
 
 (ert-deftest esh-proc-test/pipeline-connection-type/last ()
   "Test that only output streams are PTYs when a command ends a pipeline."
-  ;; Repeated unreproducible errors.
-  :tags '(:unstable)
   (skip-unless (executable-find "sh"))
   (eshell-command-result-equal
    (concat "echo | " esh-proc-test--detect-pty-cmd)
-- 
2.25.1

Re: esh-proc test failures

[Eli Zaretskii]

> Cc: larsi@gnus.org, emacs-devel@gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> Date: Mon, 22 Aug 2022 20:53:47 -0700
> 
> >> The only thing that really needs to happen here is that the signal is
> >> caught so it doesn't bubble up past this point and break things.
> > 
> > How do you accomplish that?  On MS-Windows there's no SIGPIPE signal,
> > for example.
> 
> If Eshell tries to write to a process object and it fails, it gets 
> treated as a broken pipe. Technically, it signals 'eshell-pipe-broken', 
> but that's roughly equivalent to SIGPIPE. This is mainly so that in a 
> pipeline like "foo | bar", if "bar" terminates, Eshell can inform "foo" 
> of the fact; on POSIX systems, it would send SIGPIPE, but on MS Windows, 
> it just calls 'delete-process'. This is important because we want to be 
> sure that if you have a pipeline like "yes | head", the "yes" gets 
> stopped once "head" is done.

So you basically assume that _any_ problem in the pipe was due to
SIGPIPE?  That could be too optimistic: it could be due to something
much more mundane and unrelated.

> > Not sure I understand completely what you are saying here, but AFAIR
> > writing to a closed pipe on MS-Windows will cause EINVAL errno.
> 
> Indeed, it would be nice if we could force things so that an MS Windows 
> program gets EINVAL for its WriteFile call, but because Eshell only 
> interacts indirectly with the program's output, it's too late to do that 
> by the time Eshell responds.

I don't think I follow.  When the write fails, we get an error and
propagate it all the way up to the caller.

> >> I could expand the comment to explain that this value is
> >> somewhat-arbitrary and just designed to match GNU/Linux.
> > 
> > Yes, please.
> 
> How does the attached look?

Looks OK, but are you sure about the "128" part? shouldn't it be 256
instead?  And perhaps explain why you add 128 (or whatever).

> Well, it depends on what we think users would expect. Currently, I don't 
> think Eshell provides the necessary functionality to tell when the 
> process "foo" fails (i.e. returns a non-zero exit status) in the 
> pipeline "foo | bar".

But we can do that, right?  The information about the exit status is
available to the SIGCHLD handler or a similar interface.

Re: esh-proc test failures

[Jim Porter]

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

On 8/23/2022 4:37 AM, Eli Zaretskii wrote:
>> Cc: larsi@gnus.org, emacs-devel@gnu.org
>> From: Jim Porter <jporterbugs@gmail.com>
>> Date: Mon, 22 Aug 2022 20:53:47 -0700
>>
>> If Eshell tries to write to a process object and it fails, it gets
>> treated as a broken pipe. Technically, it signals 'eshell-pipe-broken',
>> but that's roughly equivalent to SIGPIPE. This is mainly so that in a
>> pipeline like "foo | bar", if "bar" terminates, Eshell can inform "foo"
>> of the fact; on POSIX systems, it would send SIGPIPE, but on MS Windows,
>> it just calls 'delete-process'. This is important because we want to be
>> sure that if you have a pipeline like "yes | head", the "yes" gets
>> stopped once "head" is done.
> 
> So you basically assume that _any_ problem in the pipe was due to
> SIGPIPE?  That could be too optimistic: it could be due to something
> much more mundane and unrelated.

(Technically Eshell assumes that any error raised from 
'process-send-string' should *break* the pipe.) To be more cautious, we 
could say that we should only break the pipe if we get an error *and* 
the process we're writing to has terminated. See attached.

>>> Not sure I understand completely what you are saying here, but AFAIR
>>> writing to a closed pipe on MS-Windows will cause EINVAL errno.
>>
>> Indeed, it would be nice if we could force things so that an MS Windows
>> program gets EINVAL for its WriteFile call, but because Eshell only
>> interacts indirectly with the program's output, it's too late to do that
>> by the time Eshell responds.
> 
> I don't think I follow.  When the write fails, we get an error and
> propagate it all the way up to the caller.

If we signal an error in a process filter, does Emacs inform the process 
that wrote that data of the error? My tests showed that it didn't, but 
maybe I was doing something wrong.

The goal here is just to tell a process that the thing it's writing to 
is gone, and that it should give up. This is more complex than usual 
because of how many steps Eshell goes through:

   some process
     ->
   'eshell-insertion-filter' process filter
     ->
   'eshell-output-object'
     ->
   'process-send-string' to another process

Then, if 'process-send-string' signals an error, we need to relay that 
back to the original process somehow so that it stops (or possibly does 
something else appropriate).

>>>> I could expand the comment to explain that this value is
>>>> somewhat-arbitrary and just designed to match GNU/Linux.
>>>
>>> Yes, please.
>>
>> How does the attached look?
> 
> Looks OK, but are you sure about the "128" part? shouldn't it be 256
> instead?  And perhaps explain why you add 128 (or whatever).

I think the idea is that it's setting the high bit of an unsigned 8-bit 
value. I'm sure that the final number (141) is right for GNU/Linux at least:

   $ yes | head; echo ${PIPESTATUS[0]} ${PIPESTATUS[1]}
   y
   ...
   141 0

I tweaked the comment to explain this number in terms of "setting the 
high bit", which I hope should be clearer.

>> Well, it depends on what we think users would expect. Currently, I don't
>> think Eshell provides the necessary functionality to tell when the
>> process "foo" fails (i.e. returns a non-zero exit status) in the
>> pipeline "foo | bar".
> 
> But we can do that, right?  The information about the exit status is
> available to the SIGCHLD handler or a similar interface.

Yes, we could. Nothing in process.c would have to change to support 
this; it's just a limitation of how Eshell is programmed. In a pipeline 
like "yes | head", the 'eshell-last-command-status' of "yes" would be 
141, but it would then get overwritten by "head"'s status (0). Eshell 
would need to track those in a list or something.

[-- Attachment #2: 0001-Handle-eshell-pipe-broken-when-evaluating-Lisp-forms.patch --]
[-- Type: text/plain, Size: 3671 bytes --]

From 67c39ab72e9d73f6ef4af7e5cf1142c4f6b2afab Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Mon, 22 Aug 2022 09:53:24 -0700
Subject: [PATCH] Handle 'eshell-pipe-broken' when evaluating Lisp forms in
 Eshell

* lisp/eshell/esh-cmd.el (eshell-exec-lisp): Handle
'eshell-pipe-broken'.

* lisp/eshell/esh-io.el (eshell-output-object-to-target): Only signal
'eshell-pipe-broken' if the process being written to has finished.

* test/lisp/eshell/esh-proc-tests.el
(esh-proc-test/pipeline-connection-type/middle)
(esh-proc-test/pipeline-connection-type/last): Remove ':unstable'.
---
 lisp/eshell/esh-cmd.el             |  9 +++++++++
 lisp/eshell/esh-io.el              | 12 +++++++++---
 test/lisp/eshell/esh-proc-tests.el |  4 ----
 3 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 2f77f3f497..a43ad77213 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -1347,6 +1347,15 @@ eshell-exec-lisp
                  (apply func-or-form args)))))
         (and result (funcall printer result))
         result)
+    (eshell-pipe-broken
+     ;; If FUNC-OR-FORM tried and failed to write some output to a
+     ;; process, it will raise an `eshell-pipe-broken' signal (this is
+     ;; analogous to SIGPIPE on POSIX systems).  In this case, set the
+     ;; command status to some non-zero value to indicate an error; to
+     ;; match GNU/Linux, we use 141, which the numeric value of
+     ;; SIGPIPE on GNU/Linux (13) with the high bit (2^7) set.
+     (setq eshell-last-command-status 141)
+     nil)
     (error
      (setq eshell-last-command-status 1)
      (let ((msg (error-message-string err)))
diff --git a/lisp/eshell/esh-io.el b/lisp/eshell/esh-io.el
index e5977c9580..d54be55c13 100644
--- a/lisp/eshell/esh-io.el
+++ b/lisp/eshell/esh-io.el
@@ -498,10 +498,16 @@ eshell-output-object-to-target
    ((eshell-processp target)
     (unless (stringp object)
       (setq object (eshell-stringify object)))
-    (condition-case nil
+    (condition-case err
         (process-send-string target object)
-      ;; If `process-send-string' raises an error, treat it as a broken pipe.
-      (error (signal 'eshell-pipe-broken (list target)))))
+      (error
+       ;; If `process-send-string' raises an error and the process has
+       ;; finished, treat it as a broken pipe.  Otherwise, just
+       ;; re-throw the signal.
+       (if (memq (process-status target)
+		 '(run stop open closed))
+           (signal (car err) (cdr err))
+         (signal 'eshell-pipe-broken (list target))))))
 
    ((consp target)
     (apply (car target) object (cdr target))))
diff --git a/test/lisp/eshell/esh-proc-tests.el b/test/lisp/eshell/esh-proc-tests.el
index 62e784e8f6..2369bb5cc0 100644
--- a/test/lisp/eshell/esh-proc-tests.el
+++ b/test/lisp/eshell/esh-proc-tests.el
@@ -74,8 +74,6 @@ esh-proc-test/pipeline-connection-type/first
 (ert-deftest esh-proc-test/pipeline-connection-type/middle ()
   "Test that all streams are pipes when a command is in the middle of a
 pipeline."
-  ;; Repeated unreproducible errors.
-  :tags '(:unstable)
   (skip-unless (and (executable-find "sh")
                     (executable-find "cat")))
   (eshell-command-result-equal
@@ -84,8 +82,6 @@ esh-proc-test/pipeline-connection-type/middle
 
 (ert-deftest esh-proc-test/pipeline-connection-type/last ()
   "Test that only output streams are PTYs when a command ends a pipeline."
-  ;; Repeated unreproducible errors.
-  :tags '(:unstable)
   (skip-unless (executable-find "sh"))
   (eshell-command-result-equal
    (concat "echo | " esh-proc-test--detect-pty-cmd)
-- 
2.25.1

Re: esh-proc test failures

[Eli Zaretskii]

> Cc: larsi@gnus.org, emacs-devel@gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> Date: Tue, 23 Aug 2022 08:57:37 -0700
> 
> > I don't think I follow.  When the write fails, we get an error and
> > propagate it all the way up to the caller.
> 
> If we signal an error in a process filter, does Emacs inform the process 
> that wrote that data of the error? My tests showed that it didn't, but 
> maybe I was doing something wrong.

Why are you talking about the process filter?  Does that filter call
process-send-string to write to the next process in the pipe?  If so,
it should inform Emacs about any errors in writing.

> The goal here is just to tell a process that the thing it's writing to 
> is gone, and that it should give up.

That cannot happen, because Eshell is in-between.  Instead, it is
Emacs (Eshell) that should see the error, and deliver a signal to the
relevant process if needed.

Re: esh-proc test failures

[Jim Porter]

On 8/23/2022 9:22 AM, Eli Zaretskii wrote:
>> Cc: larsi@gnus.org, emacs-devel@gnu.org
>> From: Jim Porter <jporterbugs@gmail.com>
>> Date: Tue, 23 Aug 2022 08:57:37 -0700
>>
>>> I don't think I follow.  When the write fails, we get an error and
>>> propagate it all the way up to the caller.
>>
>> If we signal an error in a process filter, does Emacs inform the process
>> that wrote that data of the error? My tests showed that it didn't, but
>> maybe I was doing something wrong.
> 
> Why are you talking about the process filter?  Does that filter call
> process-send-string to write to the next process in the pipe?  If so,
> it should inform Emacs about any errors in writing.

Yes. For the pipeline "foo | bar", the process filter for "foo" 
(eventually) calls 'process-send-string' for "bar". The code in Eshell 
is designed to do what you say.

The manual says, "If an error happens during execution of a filter 
function, it is caught automatically, so that it doesn’t stop the 
execution of whatever program was running when the filter function was 
started." As I understand it, since we *want* to stop execution, that 
means Eshell needs to be responsible for notifying "foo" if it failed to 
write to "bar". Eshell does that by signaling a special error 
('eshell-pipe-broken') when writing to "bar" after it terminated[1]; 
then, the process filter for "foo" can catch that and send a SIGPIPE 
signal[2] to "foo".

>> The goal here is just to tell a process that the thing it's writing to
>> is gone, and that it should give up.
> 
> That cannot happen, because Eshell is in-between.  Instead, it is
> Emacs (Eshell) that should see the error, and deliver a signal to the
> relevant process if needed.

Then I think we're in agreement, since that's what Eshell currently 
does. (My sentence above was just trying to say that, *somehow*, Eshell 
needs to be sure that the relevant process is informed of the error. 
Delivering a signal is how Eshell does it now, and I think that's ok.)

[1] That's the behavior in my new patch. Previously, it signaled 
'eshell-pipe-broken' on any write error to "bar".

[2] Or some approximation of this on systems without SIGPIPE.

Re: esh-proc test failures

[Jim Porter]

On 8/23/2022 9:38 AM, Jim Porter wrote:
> Then I think we're in agreement, since that's what Eshell currently 
> does. (My sentence above was just trying to say that, *somehow*, Eshell 
> needs to be sure that the relevant process is informed of the error. 
> Delivering a signal is how Eshell does it now, and I think that's ok.)

Merged as f9250c5ebc1730bf3bed4382549433f52f7ef9ca. Hopefully this fixes 
EMBA; if there are any further issues anyone encounters, just let me know.

Re: esh-proc test failures

[Jim Porter]

On 8/29/2022 8:18 PM, Jim Porter wrote:
> On 8/23/2022 9:38 AM, Jim Porter wrote:
>> Then I think we're in agreement, since that's what Eshell currently 
>> does. (My sentence above was just trying to say that, *somehow*, 
>> Eshell needs to be sure that the relevant process is informed of the 
>> error. Delivering a signal is how Eshell does it now, and I think 
>> that's ok.)
> 
> Merged as f9250c5ebc1730bf3bed4382549433f52f7ef9ca. Hopefully this fixes 
> EMBA; if there are any further issues anyone encounters, just let me know.

Apparently it doesn't. I've confirmed again locally that my patch does 
fix *an* issue with 'eshell-pipe-broken', but I guess it's not the same 
issue as what's on EMBA. I'll see if I can fix EMBA for real, or failing 
that, we can disable the tests again.

Re: esh-proc test failures

[Jim Porter]

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

On 8/30/2022 9:51 AM, Jim Porter wrote:
> On 8/29/2022 8:18 PM, Jim Porter wrote:
>> Merged as f9250c5ebc1730bf3bed4382549433f52f7ef9ca. Hopefully this 
>> fixes EMBA; if there are any further issues anyone encounters, just 
>> let me know.
> 
> Apparently it doesn't. I've confirmed again locally that my patch does 
> fix *an* issue with 'eshell-pipe-broken', but I guess it's not the same 
> issue as what's on EMBA. I'll see if I can fix EMBA for real, or failing 
> that, we can disable the tests again.

Ok, it looks like this is now failing only for native-comp builds. I 
think the patch I pushed *does* fix the bug, but the regression tests 
need a bit of extra help for native-comp builds.

The attached diff should make it easier for others to see what's going 
on: with "slow-echo", Eshell will always print output after the "sh" 
subprocess has finished, triggering the bug. Then, we need to set 
'debug-on-error' to nil in order to fix this for real. That's because 
Eshell uses 'condition-case-unless-debug' inside 
'eshell-condition-case'. If anyone would like to test this out locally, 
just try out the diff with native-comp on or off and with/without the 
'debug-on-error' overrides.

However, I'm a bit concerned that there's another, more-general bug 
here. Why would native-comp and non-native-comp differ here? I think 
native-comp is doing the right thing, but maybe not... does anyone have 
any ideas here?

In any case, assuming there are no objections, I'll merge a patch that 
just sets 'debug-on-error' to nil in the appropriate spots in 
esh-proc-tests.el in the next day or so. That should fix EMBA.

[-- Attachment #2: fix-tests-under-native-comp.diff --]
[-- Type: text/plain, Size: 2195 bytes --]

diff --git a/test/lisp/eshell/esh-proc-tests.el b/test/lisp/eshell/esh-proc-tests.el
index 2369bb5cc0..51c8f7fcb8 100644
--- a/test/lisp/eshell/esh-proc-tests.el
+++ b/test/lisp/eshell/esh-proc-tests.el
@@ -22,6 +22,7 @@
 (require 'ert)
 (require 'esh-mode)
 (require 'eshell)
+(require 'em-basic)                     ; FIXME: remove this
 
 (require 'eshell-tests-helpers
          (expand-file-name "eshell-tests-helpers"
@@ -35,6 +36,15 @@ esh-proc-test--detect-pty-cmd
           "if [ -t 2 ]; then echo stderr; fi"
           "'"))
 
+;; FIXME: remove this before committing vvv
+(defun eshell/slow-echo (&rest args)
+  "A version of `eshell/echo' that takes a while to print."
+  (let ((total 0))                      ; Busy-loop for a while.
+    (dotimes (i 1000000)
+      (setq total (+ total i))))
+  (eshell/echo args))
+;; FIXME: remove this before committing ^^^
+
 ;;; Tests:
 
 (ert-deftest esh-proc-test/sigpipe-exits-process ()
@@ -76,17 +86,23 @@ esh-proc-test/pipeline-connection-type/middle
 pipeline."
   (skip-unless (and (executable-find "sh")
                     (executable-find "cat")))
-  (eshell-command-result-equal
-   (concat "echo | " esh-proc-test--detect-pty-cmd " | cat")
-   nil))
+  ;; An `eshell-pipe-broken' signal might occur internally; let Eshell
+  ;; handle it!
+  (let ((debug-on-error nil))
+    (eshell-command-result-equal
+     (concat "slow-echo hi | " esh-proc-test--detect-pty-cmd " | cat")
+     nil)))
 
 (ert-deftest esh-proc-test/pipeline-connection-type/last ()
   "Test that only output streams are PTYs when a command ends a pipeline."
   (skip-unless (executable-find "sh"))
-  (eshell-command-result-equal
-   (concat "echo | " esh-proc-test--detect-pty-cmd)
-   (unless (eq system-type 'windows-nt)
-     "stdout\nstderr\n")))
+  ;; An `eshell-pipe-broken' signal might occur internally; let Eshell
+  ;; handle it!
+  (let ((debug-on-error nil))
+    (eshell-command-result-equal
+     (concat "slow-echo hi | " esh-proc-test--detect-pty-cmd)
+     (unless (eq system-type 'windows-nt)
+       "stdout\nstderr\n"))))
 
 (ert-deftest esh-proc-test/kill-pipeline ()
   "Test that killing a pipeline of processes only emits a single

Re: esh-proc test failures

[Jim Porter]

On 8/30/2022 1:56 PM, Jim Porter wrote:
> In any case, assuming there are no objections, I'll merge a patch that 
> just sets 'debug-on-error' to nil in the appropriate spots in 
> esh-proc-tests.el in the next day or so. That should fix EMBA.

Merged as 612ff133b7720de3a551b433eb8705a6a2bbc037.