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

From: Jim Porter <[email-masked]>
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. 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.
From d56f8165ac893e6992524c8be072ffaba850a019 Mon Sep 17 00:00:00 2001
From: Jim Porter <[email-masked]>
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

From a2bd4d740c099a56494b0f3e0a02f03563ddd210 Mon Sep 17 00:00:00 2001
From: Jim Porter <[email-masked]>
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

From 88fdeab114c9e746a66758a16b2fe50aaab45624 Mon Sep 17 00:00:00 2001
From: Jim Porter <[email-masked]>
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

From 6ed606ed2a0fa27078bc16001e0a918cb305b96b Mon Sep 17 00:00:00 2001
From: Jim Porter <[email-masked]>
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