GNU bug report logs - #54062
29.0.50; [PATCH] Eshell should inform processes when a pipe is broken

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 54062 in the body.
You can then email your comments to 54062 AT debbugs.gnu.org in the normal way.

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

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 29.0.50; [PATCH] Eshell should inform processes when a pipe is broken
Date: Fri, 18 Feb 2022 20:20:10 -0800

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

Consider the following shell command:

  yes | sh -c 'read NAME'

Ordinarily, you'd expect that `sh' reads a single "y", exits, and then 
the next time `yes' tries to write, it finds that the pipe was broken. 
However, that's not what happens in Eshell. Running the above and then 
calling `M-x list-processes' will show that `yes' is still running.

Attached is a patch (with a test) to fix this by telling Eshell to 
signal SIGPIPE at the appropriate time. I've also attached a couple 
small patches to clean up a few tiny issues in Eshell; if it would be 
better to split those out into a different bug, just let me know. I 
thought it might be simpler to attach them here rather than one bug for 
each.

(Note: for the test to pass, you might need to remove the .elc files 
before running the tests. Since the patch updates 
test/lisp/eshell/eshell-tests-helpers.el and there's not an easy way to 
force that to recompile before running the actual tests, it can fail in 
some cases.)

As an aside: of course, this is a somewhat contrived example. The real 
reason I care about this is to support Lisp functions in Eshell 
pipelines, so that you can run something like `git log | less', where 
`less' is a Lisp function that opens a View mode buffer. When closing 
that buffer, it would be nice to kill the `git log' process, since it 
can go on for quite a while in a big repository. My patch series for 
that feature is getting pretty unwieldy though (14 patches and 
counting), so I peeled this off to merge separately.

[0001-lisp-eshell-esh-io.el-grep-null-device-Remove-unused.patch (text/plain, attachment)]

[0002-Improve-docstrings-for-eshell-exec-lisp-and-function.patch (text/plain, attachment)]

[0003-Ensure-eshell-output-object-always-returns-nil-for-c.patch (text/plain, attachment)]

[0004-Send-SIGPIPE-to-external-Eshell-processes-if-their-o.patch (text/plain, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 54062 <at> debbugs.gnu.org
Subject: Re: bug#54062: 29.0.50;
 [PATCH] Eshell should inform processes when a pipe is broken
Date: Sat, 19 Feb 2022 10:35:33 +0200

> From: Jim Porter <jporterbugs <at> gmail.com>
> Date: Fri, 18 Feb 2022 20:20:10 -0800
> 
> Consider the following shell command:
> 
>    yes | sh -c 'read NAME'
> 
> Ordinarily, you'd expect that `sh' reads a single "y", exits, and then 
> the next time `yes' tries to write, it finds that the pipe was broken. 
> However, that's not what happens in Eshell. Running the above and then 
> calling `M-x list-processes' will show that `yes' is still running.
> 
> Attached is a patch (with a test) to fix this by telling Eshell to 
> signal SIGPIPE at the appropriate time.

SIGPIPE isn't supported on MS-Windows, so I think we should have a
fallback there for platforms that don't support SIGPIPE.

Thanks.

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 54062 <at> debbugs.gnu.org
Subject: Re: bug#54062: 29.0.50; [PATCH] Eshell should inform processes when a
 pipe is broken
Date: Sat, 19 Feb 2022 12:02:45 -0800

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

On 2/19/2022 12:35 AM, Eli Zaretskii wrote:
>> From: Jim Porter <jporterbugs <at> gmail.com>
>> Date: Fri, 18 Feb 2022 20:20:10 -0800
>>
>> Consider the following shell command:
>>
>>     yes | sh -c 'read NAME'
>>
>> Ordinarily, you'd expect that `sh' reads a single "y", exits, and then
>> the next time `yes' tries to write, it finds that the pipe was broken.
>> However, that's not what happens in Eshell. Running the above and then
>> calling `M-x list-processes' will show that `yes' is still running.
>>
>> Attached is a patch (with a test) to fix this by telling Eshell to
>> signal SIGPIPE at the appropriate time.
> 
> SIGPIPE isn't supported on MS-Windows, so I think we should have a
> fallback there for platforms that don't support SIGPIPE.

Hmm, good point. Thinking about this some more, this also won't work for 
Tramp (which only supports `interrupt-process' as far as I can tell). I 
can think of a couple possible solutions.

One option would be to call `interrupt-process' instead, since that 
works in all cases I'm aware of. This isn't quite as nice as sending 
SIGPIPE (or equivalent) to let the process handle it how it wants, but 
at least `interrupt-process' has the same default behavior as SIGPIPE 
(i.e. terminate the process).

Another way would be to add a function like `process-break-pipe' (it 
could probably use a better name) that would close the read end of the 
process's output pipe, which - if I understand the Win32 API here - 
should trigger the right behavior on MS Windows too. It should work for 
Tramp too, although Tramp might need a bit of tweaking to handle this 
case. I've attached an outline of what this could look like; it applies 
on top of my previous patches.

One caveat is that the head process (`yes' in the example), would only 
see the "broken pipe" error on the *next* write after the one where 
Eshell detected the broken pipe. That's easy enough to fix for cases 
where we can signal SIGPIPE directly, but it's probably ok in general 
too: after all, processes don't generally know exactly when a SIGPIPE 
might occur, so it occurring slightly later shouldn't cause problems. 
(In theory, the tail process should call `process-break-pipe' as soon as 
it closes, but in Eshell, the tail process doesn't know what's feeding 
it input, so it can't easily do this.)

What do you think? Is this a sensible avenue to go down? There's 
probably room to discuss what the API should look like, but I wanted to 
be sure I was on the right track before I went too far.

[process-break-pipe.patch (text/plain, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 54062 <at> debbugs.gnu.org
Subject: Re: bug#54062: 29.0.50; [PATCH] Eshell should inform processes when a
 pipe is broken
Date: Sat, 19 Feb 2022 22:19:26 +0200

> Cc: 54062 <at> debbugs.gnu.org
> From: Jim Porter <jporterbugs <at> gmail.com>
> Date: Sat, 19 Feb 2022 12:02:45 -0800
> 
> > SIGPIPE isn't supported on MS-Windows, so I think we should have a
> > fallback there for platforms that don't support SIGPIPE.
> 
> Hmm, good point. Thinking about this some more, this also won't work for 
> Tramp (which only supports `interrupt-process' as far as I can tell). I 
> can think of a couple possible solutions.
> 
> One option would be to call `interrupt-process' instead, since that 
> works in all cases I'm aware of. This isn't quite as nice as sending 
> SIGPIPE (or equivalent) to let the process handle it how it wants, but 
> at least `interrupt-process' has the same default behavior as SIGPIPE 
> (i.e. terminate the process).

Many console programs catch SIGINT, though.

Can't we terminate ("kill") the process instead?  Or maybe deleting
the process object is enough?

> Another way would be to add a function like `process-break-pipe' (it 
> could probably use a better name) that would close the read end of the 
> process's output pipe, which - if I understand the Win32 API here - 
> should trigger the right behavior on MS Windows too.

You mean, delete the process object?  That's how we close our end of
the pipe, no?

> One caveat is that the head process (`yes' in the example), would only 
> see the "broken pipe" error on the *next* write after the one where 
> Eshell detected the broken pipe. That's easy enough to fix for cases 
> where we can signal SIGPIPE directly, but it's probably ok in general 
> too: after all, processes don't generally know exactly when a SIGPIPE 
> might occur, so it occurring slightly later shouldn't cause problems. 

I don't see a problem here.  AFAIU, closing a pipe doesn't always
deliver SIGPIPE, it can instead fail the write with EPIPE.  So SIGPIPE
is not guaranteed anyway.

> (In theory, the tail process should call `process-break-pipe' as soon as 
> it closes, but in Eshell, the tail process doesn't know what's feeding 
> it input, so it can't easily do this.)

Not sure I understand: an Emacs process object always knows what's
feeding it.

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 54062 <at> debbugs.gnu.org
Subject: Re: bug#54062: 29.0.50; [PATCH] Eshell should inform processes when a
 pipe is broken
Date: Sat, 19 Feb 2022 13:18:16 -0800

On 2/19/2022 12:19 PM, Eli Zaretskii wrote:
>> Cc: 54062 <at> debbugs.gnu.org
>> From: Jim Porter <jporterbugs <at> gmail.com>
>> Date: Sat, 19 Feb 2022 12:02:45 -0800
>>
>> One option would be to call `interrupt-process' instead, since that
>> works in all cases I'm aware of. This isn't quite as nice as sending
>> SIGPIPE (or equivalent) to let the process handle it how it wants, but
>> at least `interrupt-process' has the same default behavior as SIGPIPE
>> (i.e. terminate the process).
> 
> Many console programs catch SIGINT, though.
> 
> Can't we terminate ("kill") the process instead?  Or maybe deleting
> the process object is enough?

That might work; it would definitely be better than `interrupt-process'. 
On the other hand, I think it would be nice to handle this case by 
breaking the pipe if possible, since that would be closer to how it 
works in regular shells, as I understand it.

>> Another way would be to add a function like `process-break-pipe' (it
>> could probably use a better name) that would close the read end of the
>> process's output pipe, which - if I understand the Win32 API here -
>> should trigger the right behavior on MS Windows too.
> 
> You mean, delete the process object?  That's how we close our end of
> the pipe, no?

Do you mean using `delete-process'? That works differently from how I'm 
imagining things. From reading the code, `delete-process' sends SIGKILL 
to the process group, but that means that a process that wants to do 
something special in response to SIGPIPE (or EPIPE, or ERROR_BROKEN_PIPE 
on Win32) wouldn't be able to, since that's not the signal/error it 
receives.

In my patch, `process-break-pipe' just closes the file descriptor for 
the read end of the process's stdout pipe, but otherwise doesn't do 
anything to the process. Then, when the process tries to write to stdout 
again, the OS will report (via a signal and/or an error code) that the 
pipe is broken. Since Win32's WriteFile[1] API returns ERROR_BROKEN_PIPE 
in this case, that would let MS Windows programs detect and respond to 
broken pipes in the usual way for that platform.

>> One caveat is that the head process (`yes' in the example), would only
>> see the "broken pipe" error on the *next* write after the one where
>> Eshell detected the broken pipe. That's easy enough to fix for cases
>> where we can signal SIGPIPE directly, but it's probably ok in general
>> too: after all, processes don't generally know exactly when a SIGPIPE
>> might occur, so it occurring slightly later shouldn't cause problems.
> 
> I don't see a problem here.  AFAIU, closing a pipe doesn't always
> deliver SIGPIPE, it can instead fail the write with EPIPE.  So SIGPIPE
> is not guaranteed anyway.

Agreed, I don't think this is really a problem. I just wanted to note 
that the behavior is slightly different from how someone might expect it 
to work in a regular shell. (In any case, I think SIGPIPE and EPIPE 
occur at effectively the same time, and you would check for the latter 
if you ignored SIGPIPE, for example.[2] Maybe this comes with some 
caveats or is specific to glibc though.)

>> (In theory, the tail process should call `process-break-pipe' as soon as
>> it closes, but in Eshell, the tail process doesn't know what's feeding
>> it input, so it can't easily do this.)
> 
> Not sure I understand: an Emacs process object always knows what's
> feeding it.

Emacs process objects could probably do it, but I'm not sure if Eshell's 
pipelines are able to without being reworked. Eshell pipelines are 
assembled pretty indirectly; the output of one process goes through a 
process-filter and into `eshell-output-object', which looks up where to 
send the data in `eshell-current-handles' (which in turn is let-bound so 
each process has its own copy). It would probably take quite a bit of 
work for a process to figure out what's feeding it from its 
process-sentinel, since that happens in a different context than where 
the pipeline is constructed. Maybe it's feasible, but if we agree that 
my caveat in the section above isn't a problem, it would probably be 
simpler to avoid the extra effort.

[1] 
https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-writefile
[2] 
https://www.gnu.org/software/libc/manual/html_node/Operation-Error-Signals.html

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 54062 <at> debbugs.gnu.org
Subject: Re: bug#54062: 29.0.50; [PATCH] Eshell should inform processes when a
 pipe is broken
Date: Sun, 20 Feb 2022 09:27:08 +0200

> Cc: 54062 <at> debbugs.gnu.org
> From: Jim Porter <jporterbugs <at> gmail.com>
> Date: Sat, 19 Feb 2022 13:18:16 -0800
> 
> > Many console programs catch SIGINT, though.
> > 
> > Can't we terminate ("kill") the process instead?  Or maybe deleting
> > the process object is enough?
> 
> That might work; it would definitely be better than `interrupt-process'. 
> On the other hand, I think it would be nice to handle this case by 
> breaking the pipe if possible, since that would be closer to how it 
> works in regular shells, as I understand it.

I meant killing the process as fallback for when SIGPIPE is not
supported.

> >> Another way would be to add a function like `process-break-pipe' (it
> >> could probably use a better name) that would close the read end of the
> >> process's output pipe, which - if I understand the Win32 API here -
> >> should trigger the right behavior on MS Windows too.
> > 
> > You mean, delete the process object?  That's how we close our end of
> > the pipe, no?
> 
> Do you mean using `delete-process'? That works differently from how I'm 
> imagining things. From reading the code, `delete-process' sends SIGKILL 
> to the process group, but that means that a process that wants to do 
> something special in response to SIGPIPE (or EPIPE, or ERROR_BROKEN_PIPE 
> on Win32) wouldn't be able to, since that's not the signal/error it 
> receives.

How else can you close the pipe without deleting the process?  How can
Emacs have a process whose I/O channels aren't ready to be used?

I thought you were talking about a pipe process (make-pipe-process),
in which case deleting it closes the pipe.  But you seem to mean
something else, so now I'm not sure I understand.

> In my patch, `process-break-pipe' just closes the file descriptor for 
> the read end of the process's stdout pipe, but otherwise doesn't do 
> anything to the process.

I don't think this is a good idea.  A process isn't supposed to be in
this state.

> Then, when the process tries to write to stdout 
> again, the OS will report (via a signal and/or an error code) that the 
> pipe is broken. Since Win32's WriteFile[1] API returns ERROR_BROKEN_PIPE 
> in this case, that would let MS Windows programs detect and respond to 
> broken pipes in the usual way for that platform.

We don't use WriteFile directly, and I wouldn't rely on EPIPE being in
errno in this case without extensive testing.

Anyway, the proposal to close the pipe of a live process object is
problematic, see above.  I hope we can come up with something
simpler.  We are talking about a niche feature here, so it is IMO
better to find a simple solution for that.

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 54062 <at> debbugs.gnu.org
Subject: Re: bug#54062: 29.0.50; [PATCH] Eshell should inform processes when a
 pipe is broken
Date: Sun, 20 Feb 2022 12:17:08 -0800

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

On 2/19/2022 11:27 PM, Eli Zaretskii wrote:
>> Cc: 54062 <at> debbugs.gnu.org
>> From: Jim Porter <jporterbugs <at> gmail.com>
>> Date: Sat, 19 Feb 2022 13:18:16 -0800
>>
>>> Many console programs catch SIGINT, though.
>>>
>>> Can't we terminate ("kill") the process instead?  Or maybe deleting
>>> the process object is enough?
>>
>> That might work; it would definitely be better than `interrupt-process'.
>> On the other hand, I think it would be nice to handle this case by
>> breaking the pipe if possible, since that would be closer to how it
>> works in regular shells, as I understand it.
> 
> I meant killing the process as fallback for when SIGPIPE is not
> supported.

Ok, I've updated the patch to do this, with a note about how the 
behavior isn't quite correct. The current patch should work in most 
real-world cases, but if someone runs into a bug with this, hopefully 
the comment will point them in the right direction.

Note: I've only attached the updated patch here; parts 1-3 didn't 
change, so they're the same as in my original message[1].

>> Do you mean using `delete-process'? That works differently from how I'm
>> imagining things. From reading the code, `delete-process' sends SIGKILL
>> to the process group, but that means that a process that wants to do
>> something special in response to SIGPIPE (or EPIPE, or ERROR_BROKEN_PIPE
>> on Win32) wouldn't be able to, since that's not the signal/error it
>> receives.
> 
> How else can you close the pipe without deleting the process?  How can
> Emacs have a process whose I/O channels aren't ready to be used?
> 
> I thought you were talking about a pipe process (make-pipe-process),
> in which case deleting it closes the pipe.  But you seem to mean
> something else, so now I'm not sure I understand.

By "pipe", I meant the pair of file descriptors created by the pipe 
function (technically, `emacs_pipe') inside `create_process'. In this 
case, the subprocess being created will get the write end of that pipe, 
while Emacs gets the read end. If Emacs closes the read end of the pipe, 
then the next time the subprocess tries to write to its end of the pipe, 
it will get an error (SIGPIPE, EPIPE, or ERROR_BROKEN_PIPE, depending on 
the situation).

That's the behavior that the subprocess would expect: if the pipe for 
its stdout fd is broken, the error should refer to that problem (e.g. 
via signalling SIGPIPE). Raising the error in a different way (e.g. via 
SIGKILL) isn't quite correct.

However, as you correctly state, this is a niche feature, and messing 
around with Emacs's process management shouldn't be done lightly. It 
would take a lot of testing to be sure my previous patch is right, 
especially since my understanding of Emacs's process.c is pretty 
rudimentary. As you suggested, I think this new implementation should be 
a reasonable fallback.

[1] https://lists.gnu.org/archive/html/bug-gnu-emacs/2022-02/msg01431.html

[0004-Send-SIGPIPE-to-external-Eshell-processes-if-their-o.patch (text/plain, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 54062 <at> debbugs.gnu.org
Subject: Re: bug#54062: 29.0.50; [PATCH] Eshell should inform processes when a
 pipe is broken
Date: Mon, 21 Feb 2022 19:15:54 +0200

> Cc: 54062 <at> debbugs.gnu.org
> From: Jim Porter <jporterbugs <at> gmail.com>
> Date: Sun, 20 Feb 2022 12:17:08 -0800
> 
> Ok, I've updated the patch to do this, with a note about how the 
> behavior isn't quite correct. The current patch should work in most 
> real-world cases, but if someone runs into a bug with this, hopefully 
> the comment will point them in the right direction.

Thanks; I have no further comments.

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Jim Porter <jporterbugs <at> gmail.com>, 54062 <at> debbugs.gnu.org
Subject: Re: bug#54062: 29.0.50; [PATCH] Eshell should inform processes when
 a pipe is broken
Date: Mon, 21 Feb 2022 18:39:16 +0100

Eli Zaretskii <eliz <at> gnu.org> writes:

> Thanks; I have no further comments.

So I've pushed the patch series to Emacs 29.

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

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: jporterbugs <at> gmail.com, 54062 <at> debbugs.gnu.org
Subject: Re: bug#54062: 29.0.50; [PATCH] Eshell should inform processes when
 a pipe is broken
Date: Mon, 21 Feb 2022 20:31:15 +0200

> From: Lars Ingebrigtsen <larsi <at> gnus.org>
> Cc: Jim Porter <jporterbugs <at> gmail.com>,  54062 <at> debbugs.gnu.org
> Date: Mon, 21 Feb 2022 18:39:16 +0100
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> > Thanks; I have no further comments.
> 
> So I've pushed the patch series to Emacs 29.

Something's amiss here: the new test fails on MS-Windows because it
signals an error inside eshell-wait-for-subprocess:

  Test esh-proc-test/sigpipe-exits-process backtrace:
    signal(eshell-pipe-broken #<process sh.exe>)
    eshell-output-object-to-target("killed\n" #<process sh.exe>)
    eshell-output-object("killed\n" nil [nil (nil . 0) (nil . 0)])
    #f(compiled-function () #<bytecode 0x1c2b272f8d5f2c5e>)()
    apply(#f(compiled-function () #<bytecode 0x1c2b272f8d5f2c5e>) nil)
    timer-event-handler([t 25107 54636 874750 nil #f(compiled-function (
    sleep-for(0.1)
    sit-for(0.1)
    (while (if all eshell-process-list (eshell-interactive-process-p)) (
    (let ((start (current-time))) (while (if all eshell-process-list (es
    eshell-wait-for-subprocess(t)

Sounds like the shell is already dead/killed when
eshell-wait-for-subprocess tries to send it a string?

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>, Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 54062 <at> debbugs.gnu.org
Subject: Re: bug#54062: 29.0.50; [PATCH] Eshell should inform processes when a
 pipe is broken
Date: Mon, 21 Feb 2022 12:37:59 -0800

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

On 2/21/2022 10:31 AM, Eli Zaretskii wrote:
>> From: Lars Ingebrigtsen <larsi <at> gnus.org>
>> Cc: Jim Porter <jporterbugs <at> gmail.com>,  54062 <at> debbugs.gnu.org
>> Date: Mon, 21 Feb 2022 18:39:16 +0100
>>
>> Eli Zaretskii <eliz <at> gnu.org> writes:
>>
>>> Thanks; I have no further comments.
>>
>> So I've pushed the patch series to Emacs 29.
> 
> Something's amiss here: the new test fails on MS-Windows because it
> signals an error inside eshell-wait-for-subprocess:
> 
>    Test esh-proc-test/sigpipe-exits-process backtrace:
>      signal(eshell-pipe-broken #<process sh.exe>)
>      eshell-output-object-to-target("killed\n" #<process sh.exe>)
>      eshell-output-object("killed\n" nil [nil (nil . 0) (nil . 0)])
>      #f(compiled-function () #<bytecode 0x1c2b272f8d5f2c5e>)()
>      apply(#f(compiled-function () #<bytecode 0x1c2b272f8d5f2c5e>) nil)
>      timer-event-handler([t 25107 54636 874750 nil #f(compiled-function (
>      sleep-for(0.1)
>      sit-for(0.1)
>      (while (if all eshell-process-list (eshell-interactive-process-p)) (
>      (let ((start (current-time))) (while (if all eshell-process-list (es
>      eshell-wait-for-subprocess(t)
> 
> Sounds like the shell is already dead/killed when
> eshell-wait-for-subprocess tries to send it a string?

Thanks for merging, and sorry about the bustage. This turned out to be 
because `eshell-sentinel' for the "head" process in the pipeline called 
`eshell-output-object', but because the tail process was already dead, 
it raised `eshell-pipe-broken'. I believe the reason this only 
manifested on MS Windows was due to a timing difference between 
`delete-process' and `signal-process'; using the `delete-process' path 
on GNU/Linux shows the same problem.

Attached is a patch that ignores the `eshell-pipe-broken' error in 
`eshell-sentinel'. It's not really an error in that case anyway, since 
we only want to write the last bit of output *if we can*.

--------------------

There's just one problem remaining: when running the following command 
on MS Windows[1], you'll (usually) see two Eshell prompts get emitted 
after it finishes:

  yes | sh -c 'read NAME'

However, this is a separate bug that appears in Emacs 27.2 as well. It 
can happen whenever multiple commands in a pipeline get killed. For example:

  ~ $ sh -c 'while true; do sleep 1; echo y; done' | sh -c 'while true; 
do read NAME; echo ${NAME}; done'
  C-c C-c  ; Call `eshell-interrupt-process'

The same happens with C-c C-k (`eshell-kill-process') too. I'll file 
another bug about this, but I wanted to mention it here so no one's 
surprised if they see this come up when testing out this patch.

[1] Well, I assume it's a problem on MS Windows. I actually tested the 
MS Windows code path on GNU/Linux.

[0001-Ignore-eshell-broken-pipe-error-in-eshell-sentinel.patch (text/plain, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: larsi <at> gnus.org, 54062 <at> debbugs.gnu.org
Subject: Re: bug#54062: 29.0.50; [PATCH] Eshell should inform processes when a
 pipe is broken
Date: Tue, 22 Feb 2022 15:09:13 +0200

> Cc: 54062 <at> debbugs.gnu.org
> From: Jim Porter <jporterbugs <at> gmail.com>
> Date: Mon, 21 Feb 2022 12:37:59 -0800
> 
> Attached is a patch that ignores the `eshell-pipe-broken' error in 
> `eshell-sentinel'. It's not really an error in that case anyway, since 
> we only want to write the last bit of output *if we can*.

Thanks, this fixes the test.  However, I'm unsure we should fix this
inside eshell-sentinel: do we always want to ignore "broken pipe"
errors in Eshell subprocesses, and never show them to the user?

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: larsi <at> gnus.org, 54062 <at> debbugs.gnu.org
Subject: Re: bug#54062: 29.0.50; [PATCH] Eshell should inform processes when a
 pipe is broken
Date: Tue, 22 Feb 2022 08:49:06 -0800

On 2/22/2022 5:09 AM, Eli Zaretskii wrote:
>> Cc: 54062 <at> debbugs.gnu.org
>> From: Jim Porter <jporterbugs <at> gmail.com>
>> Date: Mon, 21 Feb 2022 12:37:59 -0800
>>
>> Attached is a patch that ignores the `eshell-pipe-broken' error in
>> `eshell-sentinel'. It's not really an error in that case anyway, since
>> we only want to write the last bit of output *if we can*.
> 
> Thanks, this fixes the test.  However, I'm unsure we should fix this
> inside eshell-sentinel: do we always want to ignore "broken pipe"
> errors in Eshell subprocesses, and never show them to the user?

I think we do want to ignore that error here. In `eshell-sentinel', we 
only run the `finish-io' code when the subprocess's state has already 
changed; in this case, that means the subprocess has already been 
terminated, since Eshell doesn't handle cases like SIGSTOP or SIGCONT 
yet (see the commented out functions at the bottom of 
lisp/eshell/esh-proc.el). Normally, if we detect a broken pipe, we'd 
want to signal the subprocess that tried to write, but since we know 
it's already been terminated, there's no (living) process to signal anymore.

It would be good to support cases like SIGSTOP/SIGCONT in the future, 
but `eshell-sentinel' already fails to account for that, so this patch 
doesn't make things worse in that regard. For example, this function 
always calls `eshell-remove-process-entry', whose docstring says:

  Record the process ENTRY as fully completed.

That's definitely not right for a process being continued with SIGCONT, 
and probably isn't right for SIGSTOP either.

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 54062 <at> debbugs.gnu.org
Subject: Re: bug#54062: 29.0.50; [PATCH] Eshell should inform processes when
 a pipe is broken
Date: Wed, 23 Feb 2022 13:14:18 +0100

Jim Porter <jporterbugs <at> gmail.com> writes:

>> Thanks, this fixes the test.  However, I'm unsure we should fix this
>> inside eshell-sentinel: do we always want to ignore "broken pipe"
>> errors in Eshell subprocesses, and never show them to the user?
>
> I think we do want to ignore that error here. In `eshell-sentinel', we
> only run the `finish-io' code when the subprocess's state has already
> changed; in this case, that means the subprocess has already been
> terminated, since Eshell doesn't handle cases like SIGSTOP or SIGCONT
> yet (see the commented out functions at the bottom of
> lisp/eshell/esh-proc.el). Normally, if we detect a broken pipe, we'd
> want to signal the subprocess that tried to write, but since we know
> it's already been terminated, there's no (living) process to signal
> anymore.

Makes sense to me, I think, so I pushed the patch to Emacs 29.

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

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 54062 <at> debbugs.gnu.org
Subject: Re: bug#54062: 29.0.50; [PATCH] Eshell should inform processes when a
 pipe is broken
Date: Wed, 23 Feb 2022 21:20:03 -0800

On 2/23/2022 4:14 AM, Lars Ingebrigtsen wrote:
> Makes sense to me, I think, so I pushed the patch to Emacs 29.

Thanks.

As mentioned upthread, I found a loosely-related issue while working on 
this. I've filed that as bug#54136, with a patch. (Just mentioning it 
here so anyone looking at this bug in the future knows the status of 
that issue.)

Previous Next

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