Re: esh-proc test failures

[Jim Porter <jporterbugs@gmail.com>]

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

On 8/22/2022 7:27 PM, Eli Zaretskii wrote:
>> Cc: larsi@gnus.org, emacs-devel@gnu.org
>> From: Jim Porter <jporterbugs@gmail.com>
>> Date: Mon, 22 Aug 2022 12:23:37 -0700
>>
>>>> +    (eshell-pipe-broken
>>>> +     ;; 141 is 128 + 13 (the numeric value of SIGPIPE).
>>>> +     (setq eshell-last-command-status 141)
>>>> +     nil)
>>>
>>> This is non-portable, I think on two counts:
>>>
>>>    . the assumption that the exit code is the signal number left-shifted
>>>      by N bits (btw, isn't N = 8, not 7?)
>>>    . the assumption that SIGPIPE is signal 13 (does Posix mandate that?)
>>>
>>> What do we expect to happen here on MS-Windows and other non-Posix
>>> platforms, where both of the above assumptions are false?
>>
>> The only thing that really needs to happen here is that the signal is
>> caught so it doesn't bubble up past this point and break things.
> 
> How do you accomplish that?  On MS-Windows there's no SIGPIPE signal,
> for example.

If Eshell tries to write to a process object and it fails, it gets 
treated as a broken pipe. Technically, it signals 'eshell-pipe-broken', 
but that's roughly equivalent to SIGPIPE. This is mainly so that in a 
pipeline like "foo | bar", if "bar" terminates, Eshell can inform "foo" 
of the fact; on POSIX systems, it would send SIGPIPE, but on MS Windows, 
it just calls 'delete-process'. This is important because we want to be 
sure that if you have a pipeline like "yes | head", the "yes" gets 
stopped once "head" is done.

Eshell's implementation isn't perfect though; since Emacs uses process 
filters, Eshell actually sends the SIGPIPE later than otherwise 
expected. Still, it's probably better than hanging indefinitely. (Fixing 
this "properly" would probably require dramatically reworking how Emacs 
interacts with subprocesses, which I think would be too risky to do, 
since we don't want to break things in process.c.)

For the actual code that raises this signal, see the end of the function 
'eshell-output-object-to-target' in lisp/eshell/esh-io.el.

>> The
>> command status could be anything really, and I'm pretty sure Eshell
>> doesn't even allow inspecting this value (yet), since it would only
>> occur for a non-last item in a pipeline. (In the future, Eshell could
>> offer something like $PIPESTATUS to examine this.)
> 
> Not sure I understand completely what you are saying here, but AFAIR
> writing to a closed pipe on MS-Windows will cause EINVAL errno.

Indeed, it would be nice if we could force things so that an MS Windows 
program gets EINVAL for its WriteFile call, but because Eshell only 
interacts indirectly with the program's output, it's too late to do that 
by the time Eshell responds.

>> I could expand the comment to explain that this value is
>> somewhat-arbitrary and just designed to match GNU/Linux.
> 
> Yes, please.

How does the attached look?

>> Alternately, if there's a way to inspect the system's conventions to
>> use here (e.g.  getting the numeric value of SIGPIPE for the current
>> system), we could do that too.
> 
> I might be able to help if I understand better what is needed here.

Well, it depends on what we think users would expect. Currently, I don't 
think Eshell provides the necessary functionality to tell when the 
process "foo" fails (i.e. returns a non-zero exit status) in the 
pipeline "foo | bar".

However, if it did, I think the most a user would care about is that 
there's some consistent way of telling what went wrong. If there were a 
way to determine what convention the current system uses to say "foo 
terminated because it tried to write to bar after bar exited", we could 
use that.

I'm not sure this part is worth spending a lot of time on though, 
especially since the exit status of "foo" isn't currently accessible as 
far as I know.

[-- Attachment #2: 0001-Handle-eshell-pipe-broken-when-evaluating-Lisp-forms.patch --]
[-- Type: text/plain, Size: 2489 bytes --]

From 69f84b4101313abc025189b2dbdc57d9b6e4849e Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Mon, 22 Aug 2022 09:53:24 -0700
Subject: [PATCH] Handle 'eshell-pipe-broken' when evaluating Lisp forms in
 Eshell

* lisp/eshell/esh-cmd.el (eshell-exec-lisp): Handle
'eshell-pipe-broken'.

* test/lisp/eshell/esh-proc-tests.el
(esh-proc-test/pipeline-connection-type/middle)
(esh-proc-test/pipeline-connection-type/last): Remove ':unstable'.
---
 lisp/eshell/esh-cmd.el             | 9 +++++++++
 test/lisp/eshell/esh-proc-tests.el | 4 ----
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 2f77f3f497..3886a1c086 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -1347,6 +1347,15 @@ eshell-exec-lisp
                  (apply func-or-form args)))))
         (and result (funcall printer result))
         result)
+    (eshell-pipe-broken
+     ;; If FUNC-OR-FORM tried and failed to write some output to a
+     ;; process, it will raise an `eshell-pipe-broken' signal (this is
+     ;; analogous to SIGPIPE on POSIX systems).  In this case, set the
+     ;; command status to some non-zero value to indicate an error; to
+     ;; match GNU/Linux, we use 141, which is 128 + 13 (the numeric
+     ;; value of SIGPIPE on GNU/Linux).
+     (setq eshell-last-command-status 141)
+     nil)
     (error
      (setq eshell-last-command-status 1)
      (let ((msg (error-message-string err)))
diff --git a/test/lisp/eshell/esh-proc-tests.el b/test/lisp/eshell/esh-proc-tests.el
index 62e784e8f6..2369bb5cc0 100644
--- a/test/lisp/eshell/esh-proc-tests.el
+++ b/test/lisp/eshell/esh-proc-tests.el
@@ -74,8 +74,6 @@ esh-proc-test/pipeline-connection-type/first
 (ert-deftest esh-proc-test/pipeline-connection-type/middle ()
   "Test that all streams are pipes when a command is in the middle of a
 pipeline."
-  ;; Repeated unreproducible errors.
-  :tags '(:unstable)
   (skip-unless (and (executable-find "sh")
                     (executable-find "cat")))
   (eshell-command-result-equal
@@ -84,8 +82,6 @@ esh-proc-test/pipeline-connection-type/middle
 
 (ert-deftest esh-proc-test/pipeline-connection-type/last ()
   "Test that only output streams are PTYs when a command ends a pipeline."
-  ;; Repeated unreproducible errors.
-  :tags '(:unstable)
   (skip-unless (executable-find "sh"))
   (eshell-command-result-equal
    (concat "echo | " esh-proc-test--detect-pty-cmd)
-- 
2.25.1