bug#54062: 29.0.50; [PATCH] Eshell should inform processes when a pipe is broken

[Jim Porter <jporterbugs@gmail.com>]

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

On 2/19/2022 12:35 AM, Eli Zaretskii wrote:
>> From: Jim Porter <jporterbugs@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.

[-- Attachment #2: process-break-pipe.patch --]
[-- Type: text/plain, Size: 1844 bytes --]

diff --git a/lisp/eshell/esh-proc.el b/lisp/eshell/esh-proc.el
index 86ae69978f..b833657c46 100644
--- a/lisp/eshell/esh-proc.el
+++ b/lisp/eshell/esh-proc.el
@@ -389,7 +389,12 @@ eshell-insertion-filter
                 (unwind-protect
                     (condition-case nil
                         (eshell-output-object data nil (cadr entry))
-                      (eshell-pipe-broken (signal-process proc 'SIGPIPE)))
+                      (eshell-pipe-broken
+                       ;; Note: this will trigger a broken pipe error
+                       ;; for the process the *next* time it tries to
+                       ;; write.  We could also opportunistically send
+                       ;; SIGPIPE here to notify the process sooner.
+                       (process-break-pipe proc)))
                   (setcar (nthcdr 4 entry) nil))))))))))
 
 (defun eshell-sentinel (proc string)
diff --git a/src/process.c b/src/process.c
index 94cc880097..12cd395cf3 100644
--- a/src/process.c
+++ b/src/process.c
@@ -2297,6 +2297,16 @@ create_pty (Lisp_Object process)
   p->pid = -2;
 }
 
+DEFUN ("process-break-pipe", Fprocess_break_up, Sprocess_break_pipe,
+       1, 1, 0,
+       doc: /* Close the read end of the process's output pipe. */)
+  (register Lisp_Object process)
+{
+  CHECK_PROCESS (process);
+  close_process_fd (&XPROCESS (process)->open_fd[READ_FROM_SUBPROCESS]);
+  return Qnil;
+}
+
 DEFUN ("make-pipe-process", Fmake_pipe_process, Smake_pipe_process,
        0, MANY, 0,
        doc: /* Create and return a bidirectional pipe process.
@@ -8615,6 +8625,7 @@ syms_of_process (void)
   defsubr (&Sset_process_buffer);
   defsubr (&Sprocess_buffer);
   defsubr (&Sprocess_mark);
+  defsubr (&Sprocess_break_pipe);
   defsubr (&Sset_process_filter);
   defsubr (&Sprocess_filter);
   defsubr (&Sset_process_sentinel);