Re: esh-proc test failures

[Jim Porter <jporterbugs@gmail.com>]

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

On 8/23/2022 4:37 AM, Eli Zaretskii wrote:
>> Cc: larsi@gnus.org, emacs-devel@gnu.org
>> From: Jim Porter <jporterbugs@gmail.com>
>> Date: Mon, 22 Aug 2022 20:53:47 -0700
>>
>> 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.
> 
> So you basically assume that _any_ problem in the pipe was due to
> SIGPIPE?  That could be too optimistic: it could be due to something
> much more mundane and unrelated.

(Technically Eshell assumes that any error raised from 
'process-send-string' should *break* the pipe.) To be more cautious, we 
could say that we should only break the pipe if we get an error *and* 
the process we're writing to has terminated. See attached.

>>> 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 don't think I follow.  When the write fails, we get an error and
> propagate it all the way up to the caller.

If we signal an error in a process filter, does Emacs inform the process 
that wrote that data of the error? My tests showed that it didn't, but 
maybe I was doing something wrong.

The goal here is just to tell a process that the thing it's writing to 
is gone, and that it should give up. This is more complex than usual 
because of how many steps Eshell goes through:

   some process
     ->
   'eshell-insertion-filter' process filter
     ->
   'eshell-output-object'
     ->
   'process-send-string' to another process

Then, if 'process-send-string' signals an error, we need to relay that 
back to the original process somehow so that it stops (or possibly does 
something else appropriate).

>>>> 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?
> 
> Looks OK, but are you sure about the "128" part? shouldn't it be 256
> instead?  And perhaps explain why you add 128 (or whatever).

I think the idea is that it's setting the high bit of an unsigned 8-bit 
value. I'm sure that the final number (141) is right for GNU/Linux at least:

   $ yes | head; echo ${PIPESTATUS[0]} ${PIPESTATUS[1]}
   y
   ...
   141 0

I tweaked the comment to explain this number in terms of "setting the 
high bit", which I hope should be clearer.

>> 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".
> 
> But we can do that, right?  The information about the exit status is
> available to the SIGCHLD handler or a similar interface.

Yes, we could. Nothing in process.c would have to change to support 
this; it's just a limitation of how Eshell is programmed. In a pipeline 
like "yes | head", the 'eshell-last-command-status' of "yes" would be 
141, but it would then get overwritten by "head"'s status (0). Eshell 
would need to track those in a list or something.

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

From 67c39ab72e9d73f6ef4af7e5cf1142c4f6b2afab 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'.

* lisp/eshell/esh-io.el (eshell-output-object-to-target): Only signal
'eshell-pipe-broken' if the process being written to has finished.

* 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 +++++++++
 lisp/eshell/esh-io.el              | 12 +++++++++---
 test/lisp/eshell/esh-proc-tests.el |  4 ----
 3 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 2f77f3f497..a43ad77213 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 the numeric value of
+     ;; SIGPIPE on GNU/Linux (13) with the high bit (2^7) set.
+     (setq eshell-last-command-status 141)
+     nil)
     (error
      (setq eshell-last-command-status 1)
      (let ((msg (error-message-string err)))
diff --git a/lisp/eshell/esh-io.el b/lisp/eshell/esh-io.el
index e5977c9580..d54be55c13 100644
--- a/lisp/eshell/esh-io.el
+++ b/lisp/eshell/esh-io.el
@@ -498,10 +498,16 @@ eshell-output-object-to-target
    ((eshell-processp target)
     (unless (stringp object)
       (setq object (eshell-stringify object)))
-    (condition-case nil
+    (condition-case err
         (process-send-string target object)
-      ;; If `process-send-string' raises an error, treat it as a broken pipe.
-      (error (signal 'eshell-pipe-broken (list target)))))
+      (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.
+       (if (memq (process-status target)
+		 '(run stop open closed))
+           (signal (car err) (cdr err))
+         (signal 'eshell-pipe-broken (list target))))))
 
    ((consp target)
     (apply (car target) object (cdr target))))
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