bug#66186: "make lisp/eshell/esh-proc-tests" fails intermittently since 7e50861ca7ed3f620fe62ac6572f6e88b3600ece

[Jim Porter <jporterbugs@gmail.com>]

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

On 9/24/2023 11:47 PM, Eli Zaretskii wrote:
>> Date: Sun, 24 Sep 2023 22:47:58 -0700
>> From: Jim Porter <jporterbugs@gmail.com>
>> Cc: jschmidt4gnu@vodafonemail.de, 66186@debbugs.gnu.org
>>
>> I forgot to add: Is there potential for a race condition here? I think
>> I'd written it the other way because there's a chance that the process
>> exits in between checking 'process-live-p' and calling
>> 'process-send-string'.
> 
> Yes, and therefore I think you should also keep the old code that
> wrapped the call in condition-case.

Ok, so I've rewritten the patch. Now there are no non-test code changes, 
so Eshell works as it did before, for better or worse. Jumping through 
hoops to reduce, but not eliminate, the chance of a crash didn't seem 
like the right direction to me.

However, I also added a comment in 'eshell-output-object-to-target' 
pointing to this bug, in case anyone finds this SIGPIPE behavior to be 
an actual problem (it might be an issue for people who want to write 
shell scripts in Eshell, but I don't think that's very common anyway). 
And then...

>> I could probably also write the test to avoid this race condition
>> entirely, since it's not actually trying to trigger a SIGPIPE (though in
>> general, Eshell should do the right thing in response to SIGPIPE). That
>> would make the regression tests happy.
> 
> That's always a good thing, thanks.

... I've also done this. Now the regression tests should just avoid the 
possibility of a SIGPIPE, which will hopefully resolve this bug.

Jens, could you try this version out to make sure the tests pass 
reliably for you?

[-- Attachment #2: 0001-Adjust-Eshell-regression-tests-to-avoid-SIGPIPE.patch --]
[-- Type: text/plain, Size: 3019 bytes --]

From 2feac3f3c0a6630aadb4746c3fdcc167bda2e253 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Sun, 24 Sep 2023 22:30:34 -0700
Subject: [PATCH] ; Adjust Eshell regression tests to avoid SIGPIPE

In batch mode, SIGPIPEs can cause Emacs to abort (bug#66186).

* lisp/eshell/esh-io.el (eshell-output-object-to-target): Update
comment.

* test/lisp/eshell/esh-proc-tests.el
(esh-proc-test/pipeline-connection-type/middle)
(esh-proc-test/pipeline-connection-type/last): Use 'printnl', since
that causes no output when called with no arguments, thus avoiding a
risky 'process-send-string'.
---
 lisp/eshell/esh-io.el              |  7 +++++--
 test/lisp/eshell/esh-proc-tests.el | 20 +++++++-------------
 2 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/lisp/eshell/esh-io.el b/lisp/eshell/esh-io.el
index cd0cee6e21d..d0f1e04e925 100644
--- a/lisp/eshell/esh-io.el
+++ b/lisp/eshell/esh-io.el
@@ -648,8 +648,11 @@ eshell-output-object-to-target
       (process-send-string target object)
     (error
      ;; If `process-send-string' raises an error and the process has
-     ;; finished, treat it as a broken pipe.  Otherwise, just
-     ;; re-throw the signal.
+     ;; finished, treat it as a broken pipe.  Otherwise, just re-raise
+     ;; the signal.  NOTE: When running Emacs in batch mode
+     ;; (e.g. during regression tests), Emacs can abort due to SIGPIPE
+     ;; here.  Maybe `process-send-string' should handle SIGPIPE even
+     ;; in batch mode (bug#66186).
      (if (process-live-p target)
          (signal (car err) (cdr err))
        (signal 'eshell-pipe-broken (list target)))))
diff --git a/test/lisp/eshell/esh-proc-tests.el b/test/lisp/eshell/esh-proc-tests.el
index d58764ac29f..2f03c07b35e 100644
--- a/test/lisp/eshell/esh-proc-tests.el
+++ b/test/lisp/eshell/esh-proc-tests.el
@@ -174,23 +174,17 @@ esh-proc-test/pipeline-connection-type/middle
 pipeline."
   (skip-unless (and (executable-find "sh")
                     (executable-find "cat")))
-  ;; An `eshell-pipe-broken' signal might occur internally; let Eshell
-  ;; handle it!
-  (let ((debug-on-error nil))
-    (eshell-command-result-equal
-     (concat "echo hi | " esh-proc-test--detect-pty-cmd " | cat")
-     nil)))
+  (eshell-command-result-equal
+   (concat "printnl | " esh-proc-test--detect-pty-cmd " | cat")
+   nil))
 
 (ert-deftest esh-proc-test/pipeline-connection-type/last ()
   "Test that only output streams are PTYs when a command ends a pipeline."
   (skip-unless (executable-find "sh"))
-  ;; An `eshell-pipe-broken' signal might occur internally; let Eshell
-  ;; handle it!
-  (let ((debug-on-error nil))
-    (eshell-command-result-equal
-     (concat "echo hi | " esh-proc-test--detect-pty-cmd)
-     (unless (eq system-type 'windows-nt)
-       "stdout\nstderr\n"))))
+  (eshell-command-result-equal
+   (concat "printnl | " esh-proc-test--detect-pty-cmd)
+   (unless (eq system-type 'windows-nt)
+     "stdout\nstderr\n")))
 
 \f
 ;; Synchronous processes
-- 
2.25.1