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

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

[Jim Porter]

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

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.

[-- Attachment #2: 0001-lisp-eshell-esh-io.el-grep-null-device-Remove-unused.patch --]
[-- Type: text/plain, Size: 754 bytes --]

From d56f8165ac893e6992524c8be072ffaba850a019 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Sat, 29 Jan 2022 17:28:23 -0800
Subject: [PATCH 1/4] ; * lisp/eshell/esh-io.el (grep-null-device): Remove
 unused defvar.

---
 lisp/eshell/esh-io.el | 2 --
 1 file changed, 2 deletions(-)

diff --git a/lisp/eshell/esh-io.el b/lisp/eshell/esh-io.el
index 8e6463eac2..e457f65c18 100644
--- a/lisp/eshell/esh-io.el
+++ b/lisp/eshell/esh-io.el
@@ -375,8 +375,6 @@ eshell-get-target
     (error "Invalid redirection target: %s"
 	   (eshell-stringify target)))))
 
-(defvar grep-null-device)
-
 (defun eshell-set-output-handle (index mode &optional target)
   "Set handle INDEX, using MODE, to point to TARGET."
   (when target
-- 
2.25.1


[-- Attachment #3: 0002-Improve-docstrings-for-eshell-exec-lisp-and-function.patch --]
[-- Type: text/plain, Size: 4511 bytes --]

From a2bd4d740c099a56494b0f3e0a02f03563ddd210 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Thu, 13 Jan 2022 18:58:50 -0800
Subject: [PATCH 2/4] Improve docstrings for 'eshell-exec-lisp' and functions
 that call it

* lisp/eshell/esh-cmd.el (eshell-exec-lisp, eshell-apply*)
(eshell-funcall*, eshell-eval*, eshell-apply, eshell-eval)
(eshell-funcall, eshell-applyn, eshell-funcalln, eshell-evaln):
Improve docstrings.
---
 lisp/eshell/esh-cmd.el | 47 +++++++++++++++++++++++++++---------------
 1 file changed, 30 insertions(+), 17 deletions(-)

diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 5819506cc0..dceb061c8f 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -1292,8 +1292,9 @@ eshell-plain-command
 (defun eshell-exec-lisp (printer errprint func-or-form args form-p)
   "Execute a Lisp FUNC-OR-FORM, maybe passing ARGS.
 PRINTER and ERRPRINT are functions to use for printing regular
-messages, and errors.  FORM-P should be non-nil if FUNC-OR-FORM
-represent a Lisp form; ARGS will be ignored in that case."
+messages and errors, respectively.  FORM-P should be non-nil if
+FUNC-OR-FORM represent a Lisp form; ARGS will be ignored in that
+case."
   (eshell-condition-case err
       (let ((result
              (save-current-buffer
@@ -1316,44 +1317,56 @@ eshell-exec-lisp
 (defsubst eshell-apply* (printer errprint func args)
   "Call FUNC, with ARGS, trapping errors and return them as output.
 PRINTER and ERRPRINT are functions to use for printing regular
-messages, and errors."
+messages and errors, respectively."
   (eshell-exec-lisp printer errprint func args nil))
 
 (defsubst eshell-funcall* (printer errprint func &rest args)
-  "Call FUNC, with ARGS, trapping errors and return them as output."
+  "Call FUNC, with ARGS, trapping errors and return them as output.
+PRINTER and ERRPRINT are functions to use for printing regular
+messages and errors, respectively."
   (eshell-apply* printer errprint func args))
 
 (defsubst eshell-eval* (printer errprint form)
-  "Evaluate FORM, trapping errors and returning them."
+  "Evaluate FORM, trapping errors and returning them.
+PRINTER and ERRPRINT are functions to use for printing regular
+messages and errors, respectively."
   (eshell-exec-lisp printer errprint form nil t))
 
 (defsubst eshell-apply (func args)
   "Call FUNC, with ARGS, trapping errors and return them as output.
-PRINTER and ERRPRINT are functions to use for printing regular
-messages, and errors."
-  (eshell-apply* 'eshell-print 'eshell-error func args))
+Print the result using `eshell-print'; if an error occurs, print
+it via `eshell-error'."
+  (eshell-apply* #'eshell-print #'eshell-error func args))
 
 (defsubst eshell-funcall (func &rest args)
-  "Call FUNC, with ARGS, trapping errors and return them as output."
+  "Call FUNC, with ARGS, trapping errors and return them as output.
+Print the result using `eshell-print'; if an error occurs, print
+it via `eshell-error'."
   (eshell-apply func args))
 
 (defsubst eshell-eval (form)
-  "Evaluate FORM, trapping errors and returning them."
-  (eshell-eval* 'eshell-print 'eshell-error form))
+  "Evaluate FORM, trapping errors and returning them.
+Print the result using `eshell-print'; if an error occurs, print
+it via `eshell-error'."
+  (eshell-eval* #'eshell-print #'eshell-error form))
 
 (defsubst eshell-applyn (func args)
   "Call FUNC, with ARGS, trapping errors and return them as output.
-PRINTER and ERRPRINT are functions to use for printing regular
-messages, and errors."
-  (eshell-apply* 'eshell-printn 'eshell-errorn func args))
+Print the result using `eshell-printn'; if an error occurs, print it
+via `eshell-errorn'."
+  (eshell-apply* #'eshell-printn #'eshell-errorn func args))
 
 (defsubst eshell-funcalln (func &rest args)
-  "Call FUNC, with ARGS, trapping errors and return them as output."
+  "Call FUNC, with ARGS, trapping errors and return them as output.
+Print the result using `eshell-printn'; if an error occurs, print it
+via `eshell-errorn'."
   (eshell-applyn func args))
 
 (defsubst eshell-evaln (form)
-  "Evaluate FORM, trapping errors and returning them."
-  (eshell-eval* 'eshell-printn 'eshell-errorn form))
+  "Evaluate FORM, trapping errors and returning them.
+Print the result using `eshell-printn'; if an error occurs, print it
+via `eshell-errorn'."
+  (eshell-eval* #'eshell-printn #'eshell-errorn form))
 
 (defvar eshell-last-output-end)         ;Defined in esh-mode.el.
 
-- 
2.25.1


[-- Attachment #4: 0003-Ensure-eshell-output-object-always-returns-nil-for-c.patch --]
[-- Type: text/plain, Size: 1737 bytes --]

From 88fdeab114c9e746a66758a16b2fe50aaab45624 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Wed, 19 Jan 2022 21:57:38 -0800
Subject: [PATCH 3/4] Ensure 'eshell-output-object' always returns nil for
 consistency

This prevents functions like 'eshell-print' from writing doubled
output when run in Eshell.  Previously, the result would be:

  ~ $ eshell-print hi
  hihi

* lisp/eshell/esh-io.el (eshell-output-object): Always return nil.
---
 lisp/eshell/esh-io.el | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/lisp/eshell/esh-io.el b/lisp/eshell/esh-io.el
index e457f65c18..fc1124561a 100644
--- a/lisp/eshell/esh-io.el
+++ b/lisp/eshell/esh-io.el
@@ -491,14 +491,19 @@ eshell-output-object-to-target
   object)
 
 (defun eshell-output-object (object &optional handle-index handles)
-  "Insert OBJECT, using HANDLE-INDEX specifically)."
+  "Insert OBJECT, using HANDLE-INDEX specifically.
+If HANDLE-INDEX is nil, output to `eshell-output-handle'.
+HANDLES is the set of file handles to use; if nil, use
+`eshell-current-handles'."
   (let ((target (car (aref (or handles eshell-current-handles)
 			   (or handle-index eshell-output-handle)))))
-    (if (and target (not (listp target)))
-	(eshell-output-object-to-target object target)
-      (while target
-	(eshell-output-object-to-target object (car target))
-	(setq target (cdr target))))))
+    (if (listp target)
+        (while target
+	  (eshell-output-object-to-target object (car target))
+	  (setq target (cdr target)))
+      (eshell-output-object-to-target object target)
+      ;; Explicitly return nil to match the list case above.
+      nil)))
 
 (provide 'esh-io)
 ;;; esh-io.el ends here
-- 
2.25.1


[-- Attachment #5: 0004-Send-SIGPIPE-to-external-Eshell-processes-if-their-o.patch --]
[-- Type: text/plain, Size: 6655 bytes --]

From 6ed606ed2a0fa27078bc16001e0a918cb305b96b Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Tue, 1 Feb 2022 19:16:00 -0800
Subject: [PATCH 4/4] Send SIGPIPE to external Eshell processes if their output
 target closes

* lisp/eshell/esh-io.el (eshell-pipe-broken): New error.
(eshell-output-object-to-target): Signal 'eshell-pipe-broken' if the
target is an exited/signaled process.

* lisp/eshell/esh-proc.el (eshell-insertion-filter): Handle
'eshell-pipe-broken'.

* test/lisp/eshell/esh-proc-tests.el: New test.
---
 lisp/eshell/esh-io.el                    | 12 ++++---
 lisp/eshell/esh-proc.el                  | 15 +++++---
 test/lisp/eshell/esh-proc-tests.el       | 45 ++++++++++++++++++++++++
 test/lisp/eshell/eshell-tests-helpers.el |  9 +++--
 4 files changed, 70 insertions(+), 11 deletions(-)
 create mode 100644 test/lisp/eshell/esh-proc-tests.el

diff --git a/lisp/eshell/esh-io.el b/lisp/eshell/esh-io.el
index fc1124561a..3644c1a18b 100644
--- a/lisp/eshell/esh-io.el
+++ b/lisp/eshell/esh-io.el
@@ -150,6 +150,8 @@ eshell-virtual-targets
   :risky t
   :group 'eshell-io)
 
+(define-error 'eshell-pipe-broken "Pipe broken")
+
 ;;; Internal Variables:
 
 (defvar eshell-current-handles nil)
@@ -481,10 +483,12 @@ eshell-output-object-to-target
 		(goto-char target))))))
 
    ((eshell-processp target)
-    (when (eq (process-status target) 'run)
-      (unless (stringp object)
-       (setq object (eshell-stringify object)))
-      (process-send-string target object)))
+    (unless (stringp object)
+      (setq object (eshell-stringify object)))
+    (condition-case nil
+        (process-send-string target object)
+      ;; If `process-send-string' raises an error, treat it as a broken pipe.
+      (error (signal 'eshell-pipe-broken target))))
 
    ((consp target)
     (apply (car target) object (cdr target))))
diff --git a/lisp/eshell/esh-proc.el b/lisp/eshell/esh-proc.el
index bb2136c06c..86ae69978f 100644
--- a/lisp/eshell/esh-proc.el
+++ b/lisp/eshell/esh-proc.el
@@ -386,8 +386,11 @@ eshell-insertion-filter
 	      (let ((data (nth 3 entry)))
 		(setcar (nthcdr 3 entry) nil)
 		(setcar (nthcdr 4 entry) t)
-		(eshell-output-object data nil (cadr entry))
-		(setcar (nthcdr 4 entry) nil)))))))))
+                (unwind-protect
+                    (condition-case nil
+                        (eshell-output-object data nil (cadr entry))
+                      (eshell-pipe-broken (signal-process proc 'SIGPIPE)))
+                  (setcar (nthcdr 4 entry) nil))))))))))
 
 (defun eshell-sentinel (proc string)
   "Generic sentinel for command processes.  Reports only signals.
@@ -416,8 +419,12 @@ eshell-sentinel
                                   (lambda ()
                                     (if (nth 4 entry)
                                         (run-at-time 0 nil finish-io)
-                                      (when str (eshell-output-object str nil handles))
-                                      (eshell-close-handles status 'nil handles)))))
+                                      (unwind-protect
+                                          (when str
+                                            (eshell-output-object
+                                             str nil handles))
+                                        (eshell-close-handles
+                                         status 'nil handles))))))
                           (funcall finish-io)))))
 		(eshell-remove-process-entry entry))))
 	(eshell-kill-process-function proc string)))))
diff --git a/test/lisp/eshell/esh-proc-tests.el b/test/lisp/eshell/esh-proc-tests.el
new file mode 100644
index 0000000000..e7ea6c00d6
--- /dev/null
+++ b/test/lisp/eshell/esh-proc-tests.el
@@ -0,0 +1,45 @@
+;;; esh-proc-tests.el --- esh-proc test suite  -*- lexical-binding:t -*-
+
+;; Copyright (C) 2022 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Code:
+
+(require 'ert)
+(require 'esh-mode)
+(require 'eshell)
+
+(require 'eshell-tests-helpers
+         (expand-file-name "eshell-tests-helpers"
+                           (file-name-directory (or load-file-name
+                                                    default-directory))))
+
+(ert-deftest esh-proc-test/sigpipe-exits-process ()
+  "Test that a SIGPIPE is properly sent to a process if a pipe closes"
+  (skip-unless (and (executable-find "sh")
+                    (executable-find "echo")
+                    (executable-find "sleep")))
+  (with-temp-eshell
+   (eshell-command-result-p
+    ;; The first command is like `yes' but slower.  This is to prevent
+    ;; it from taxing Emacs's process filter too much and causing a
+    ;; hang.
+    (concat "sh -c 'while true; do echo y; sleep 1; done' | "
+            "sh -c 'read NAME; echo ${NAME}'")
+    "y\n")
+   (eshell-wait-for-subprocess t)
+   (should (eq (process-list) nil))))
diff --git a/test/lisp/eshell/eshell-tests-helpers.el b/test/lisp/eshell/eshell-tests-helpers.el
index 33cdd60113..f944194a2b 100644
--- a/test/lisp/eshell/eshell-tests-helpers.el
+++ b/test/lisp/eshell/eshell-tests-helpers.el
@@ -50,15 +50,18 @@ with-temp-eshell
          (let (kill-buffer-query-functions)
            (kill-buffer eshell-buffer))))))
 
-(defun eshell-wait-for-subprocess ()
+(defun eshell-wait-for-subprocess (&optional all)
   "Wait until there is no interactive subprocess running in Eshell.
+If ALL is non-nil, wait until there are no Eshell subprocesses at
+all running.
+
 If this takes longer than `eshell-test--max-subprocess-time',
 raise an error."
   (let ((start (current-time)))
-    (while (eshell-interactive-process-p)
+    (while (if all eshell-process-list (eshell-interactive-process-p))
       (when (> (float-time (time-since start))
                eshell-test--max-subprocess-time)
-        (error "timed out waiting for subprocess"))
+        (error "timed out waiting for subprocess(es)"))
       (sit-for 0.1))))
 
 (defun eshell-insert-command (text &optional func)
-- 
2.25.1

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

[Eli Zaretskii]

> 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.

Thanks.

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

[Jim Porter]

[-- 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);

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

[Eli Zaretskii]

> Cc: 54062@debbugs.gnu.org
> From: Jim Porter <jporterbugs@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.

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

[Jim Porter]

On 2/19/2022 12:19 PM, Eli Zaretskii wrote:
>> Cc: 54062@debbugs.gnu.org
>> From: Jim Porter <jporterbugs@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

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

[Eli Zaretskii]

> Cc: 54062@debbugs.gnu.org
> From: Jim Porter <jporterbugs@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.

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

[Jim Porter]

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

On 2/19/2022 11:27 PM, Eli Zaretskii wrote:
>> Cc: 54062@debbugs.gnu.org
>> From: Jim Porter <jporterbugs@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

[-- Attachment #2: 0004-Send-SIGPIPE-to-external-Eshell-processes-if-their-o.patch --]
[-- Type: text/plain, Size: 7658 bytes --]

From 99e73733d17f103baf039cfaf059fc0c54064191 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Tue, 1 Feb 2022 19:16:00 -0800
Subject: [PATCH 4/4] Send SIGPIPE to external Eshell processes if their output
 target closes

* lisp/eshell/esh-io.el (eshell-pipe-broken): New error.
(eshell-output-object-to-target): Signal 'eshell-pipe-broken' if the
target is an exited/signaled process.

* lisp/eshell/esh-proc.el (eshell-insertion-filter): Handle
'eshell-pipe-broken'.

* test/lisp/eshell/esh-proc-tests.el: New test.
---
 lisp/eshell/esh-io.el                    | 12 ++++---
 lisp/eshell/esh-proc.el                  | 31 +++++++++++++---
 test/lisp/eshell/esh-proc-tests.el       | 45 ++++++++++++++++++++++++
 test/lisp/eshell/eshell-tests-helpers.el |  9 +++--
 4 files changed, 86 insertions(+), 11 deletions(-)
 create mode 100644 test/lisp/eshell/esh-proc-tests.el

diff --git a/lisp/eshell/esh-io.el b/lisp/eshell/esh-io.el
index fc1124561a..3644c1a18b 100644
--- a/lisp/eshell/esh-io.el
+++ b/lisp/eshell/esh-io.el
@@ -150,6 +150,8 @@ eshell-virtual-targets
   :risky t
   :group 'eshell-io)
 
+(define-error 'eshell-pipe-broken "Pipe broken")
+
 ;;; Internal Variables:
 
 (defvar eshell-current-handles nil)
@@ -481,10 +483,12 @@ eshell-output-object-to-target
 		(goto-char target))))))
 
    ((eshell-processp target)
-    (when (eq (process-status target) 'run)
-      (unless (stringp object)
-       (setq object (eshell-stringify object)))
-      (process-send-string target object)))
+    (unless (stringp object)
+      (setq object (eshell-stringify object)))
+    (condition-case nil
+        (process-send-string target object)
+      ;; If `process-send-string' raises an error, treat it as a broken pipe.
+      (error (signal 'eshell-pipe-broken target))))
 
    ((consp target)
     (apply (car target) object (cdr target))))
diff --git a/lisp/eshell/esh-proc.el b/lisp/eshell/esh-proc.el
index bb2136c06c..ed37de85f7 100644
--- a/lisp/eshell/esh-proc.el
+++ b/lisp/eshell/esh-proc.el
@@ -386,8 +386,27 @@ eshell-insertion-filter
 	      (let ((data (nth 3 entry)))
 		(setcar (nthcdr 3 entry) nil)
 		(setcar (nthcdr 4 entry) t)
-		(eshell-output-object data nil (cadr entry))
-		(setcar (nthcdr 4 entry) nil)))))))))
+                (unwind-protect
+                    (condition-case nil
+                        (eshell-output-object data nil (cadr entry))
+                      ;; FIXME: We want to send SIGPIPE to the process
+                      ;; here.  However, remote processes don't
+                      ;; currently support that, and not all systems
+                      ;; have SIGPIPE in the first place (e.g. MS
+                      ;; Windows).  In these cases, just delete the
+                      ;; process; this is reasonably close to the
+                      ;; right behavior, since the default action for
+                      ;; SIGPIPE is to terminate the process.  For use
+                      ;; cases where SIGPIPE is truly needed, using an
+                      ;; external pipe operator (`*|') may work
+                      ;; instead (e.g. when working with remote
+                      ;; processes).
+                      (eshell-pipe-broken
+                       (if (or (process-get proc 'remote-pid)
+                               (eq system-type 'windows-nt))
+                           (delete-process proc)
+                         (signal-process proc 'SIGPIPE))))
+                  (setcar (nthcdr 4 entry) nil))))))))))
 
 (defun eshell-sentinel (proc string)
   "Generic sentinel for command processes.  Reports only signals.
@@ -416,8 +435,12 @@ eshell-sentinel
                                   (lambda ()
                                     (if (nth 4 entry)
                                         (run-at-time 0 nil finish-io)
-                                      (when str (eshell-output-object str nil handles))
-                                      (eshell-close-handles status 'nil handles)))))
+                                      (unwind-protect
+                                          (when str
+                                            (eshell-output-object
+                                             str nil handles))
+                                        (eshell-close-handles
+                                         status 'nil handles))))))
                           (funcall finish-io)))))
 		(eshell-remove-process-entry entry))))
 	(eshell-kill-process-function proc string)))))
diff --git a/test/lisp/eshell/esh-proc-tests.el b/test/lisp/eshell/esh-proc-tests.el
new file mode 100644
index 0000000000..e7ea6c00d6
--- /dev/null
+++ b/test/lisp/eshell/esh-proc-tests.el
@@ -0,0 +1,45 @@
+;;; esh-proc-tests.el --- esh-proc test suite  -*- lexical-binding:t -*-
+
+;; Copyright (C) 2022 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Code:
+
+(require 'ert)
+(require 'esh-mode)
+(require 'eshell)
+
+(require 'eshell-tests-helpers
+         (expand-file-name "eshell-tests-helpers"
+                           (file-name-directory (or load-file-name
+                                                    default-directory))))
+
+(ert-deftest esh-proc-test/sigpipe-exits-process ()
+  "Test that a SIGPIPE is properly sent to a process if a pipe closes"
+  (skip-unless (and (executable-find "sh")
+                    (executable-find "echo")
+                    (executable-find "sleep")))
+  (with-temp-eshell
+   (eshell-command-result-p
+    ;; The first command is like `yes' but slower.  This is to prevent
+    ;; it from taxing Emacs's process filter too much and causing a
+    ;; hang.
+    (concat "sh -c 'while true; do echo y; sleep 1; done' | "
+            "sh -c 'read NAME; echo ${NAME}'")
+    "y\n")
+   (eshell-wait-for-subprocess t)
+   (should (eq (process-list) nil))))
diff --git a/test/lisp/eshell/eshell-tests-helpers.el b/test/lisp/eshell/eshell-tests-helpers.el
index 33cdd60113..f944194a2b 100644
--- a/test/lisp/eshell/eshell-tests-helpers.el
+++ b/test/lisp/eshell/eshell-tests-helpers.el
@@ -50,15 +50,18 @@ with-temp-eshell
          (let (kill-buffer-query-functions)
            (kill-buffer eshell-buffer))))))
 
-(defun eshell-wait-for-subprocess ()
+(defun eshell-wait-for-subprocess (&optional all)
   "Wait until there is no interactive subprocess running in Eshell.
+If ALL is non-nil, wait until there are no Eshell subprocesses at
+all running.
+
 If this takes longer than `eshell-test--max-subprocess-time',
 raise an error."
   (let ((start (current-time)))
-    (while (eshell-interactive-process-p)
+    (while (if all eshell-process-list (eshell-interactive-process-p))
       (when (> (float-time (time-since start))
                eshell-test--max-subprocess-time)
-        (error "timed out waiting for subprocess"))
+        (error "timed out waiting for subprocess(es)"))
       (sit-for 0.1))))
 
 (defun eshell-insert-command (text &optional func)
-- 
2.25.1

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

[Eli Zaretskii]

> Cc: 54062@debbugs.gnu.org
> From: Jim Porter <jporterbugs@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.

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

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@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

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

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Jim Porter <jporterbugs@gmail.com>,  54062@debbugs.gnu.org
> Date: Mon, 21 Feb 2022 18:39:16 +0100
> 
> Eli Zaretskii <eliz@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?

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

[Jim Porter]

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

On 2/21/2022 10:31 AM, Eli Zaretskii wrote:
>> From: Lars Ingebrigtsen <larsi@gnus.org>
>> Cc: Jim Porter <jporterbugs@gmail.com>,  54062@debbugs.gnu.org
>> Date: Mon, 21 Feb 2022 18:39:16 +0100
>>
>> Eli Zaretskii <eliz@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.

[-- Attachment #2: 0001-Ignore-eshell-broken-pipe-error-in-eshell-sentinel.patch --]
[-- Type: text/plain, Size: 1805 bytes --]

From f41b9e2599fe3a1a959ef1b52f77032fd8d28c20 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Mon, 21 Feb 2022 12:14:34 -0800
Subject: [PATCH] Ignore 'eshell-broken-pipe' error in 'eshell-sentinel'

This can happen if 'eshell-sentinel' tries to write output to another
process, but that process has already terminated.

* lisp/eshell/esh-proc.el (eshell-sentinel): Use 'ignore-error'
instead of 'unwind-protect'.
---
 lisp/eshell/esh-proc.el | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/lisp/eshell/esh-proc.el b/lisp/eshell/esh-proc.el
index ed37de85f7..d7d22d2a9e 100644
--- a/lisp/eshell/esh-proc.el
+++ b/lisp/eshell/esh-proc.el
@@ -435,12 +435,12 @@ eshell-sentinel
                                   (lambda ()
                                     (if (nth 4 entry)
                                         (run-at-time 0 nil finish-io)
-                                      (unwind-protect
-                                          (when str
-                                            (eshell-output-object
-                                             str nil handles))
-                                        (eshell-close-handles
-                                         status 'nil handles))))))
+                                      (when str
+                                        (ignore-error 'eshell-pipe-broken
+                                          (eshell-output-object
+                                           str nil handles)))
+                                      (eshell-close-handles
+                                       status 'nil handles)))))
                           (funcall finish-io)))))
 		(eshell-remove-process-entry entry))))
 	(eshell-kill-process-function proc string)))))
-- 
2.25.1

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

[Eli Zaretskii]

> Cc: 54062@debbugs.gnu.org
> From: Jim Porter <jporterbugs@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?

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

[Jim Porter]

On 2/22/2022 5:09 AM, Eli Zaretskii wrote:
>> Cc: 54062@debbugs.gnu.org
>> From: Jim Porter <jporterbugs@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.

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

[Lars Ingebrigtsen]

Jim Porter <jporterbugs@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

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

[Jim Porter]

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.)