bug#56025: 29.0.50; em-extpipe-test-2 times out on EMBA and Cygwin

[Jim Porter <jporterbugs@gmail.com>]

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

On 6/23/2022 9:40 PM, Sean Whitton wrote:
 > I'm a bit queasy about an unbounded loop here.  Why not just try three
 > times?  Or, better, try twice, and a third time only if we're on a
 > platform where we know it's needed.

How about the attached patch? I didn't check for specific platforms to 
enable the "third EOF" behavior, since a) it's hard to say for sure 
which platforms might have this issue (especially since Cygwin will be 
fixing it), and b) this lets us avoid worrying about Tramp compatibility.

 > Many thanks for the investigative work.

Agreed, this turned out to be a much subtler problem than I had 
initially suspected. Thanks!

On 6/23/2022 11:07 PM, Eli Zaretskii wrote:
> Please add there comments explaining why this is done, or at least
> point to relevant messages in this bug's discussion (NOT just to the
> bug number, as the discussion is long and it will be hard to
> understand what part of it is relevant).  Such "tricky" code should
> always have comments explaining it.

I added a comment explaining this to the best of my knowledge. There's 
one additional caveat I didn't mention there though, since it's only 
somewhat related. I believe this was mentioned earlier in the thread, 
but when Eshell creates a pipe, it routes both stdout and stderr to the 
next process's stdin (there's no way to control this behavior yet). When 
closing the handles from the initial process, it then calls 
`eshell-close-target' twice: once for stdout and once for stderr. Thus, 
with this patch, we'll call `process-send-eof' up to six times.

I'm not sure this is really a problem in practice today, but it might 
come up if Eshell gains the ability to redirect stdout and stderr 
separately.

[-- Attachment #2: 0001-When-closing-an-Eshell-process-target-send-EOF-three.patch --]
[-- Type: text/plain, Size: 2419 bytes --]

From a5f8dc8af6d199be8e3b09803835efa9d70b76a6 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Fri, 24 Jun 2022 09:14:38 -0700
Subject: [PATCH] When closing an Eshell process target, send EOF three times

* lisp/eshell/esh-io.el (eshell-close-target): Send EOF 3 times.

* test/lisp/eshell/em-extpipe-tests.el (em-extpipe-tests--deftest):
Re-enable these tests on EMBA.

This patch is adapted by one from Ken Brown, who uncovered the reason
for this bug (bug#56025).
---
 lisp/eshell/esh-io.el                | 15 +++++++++++++--
 test/lisp/eshell/em-extpipe-tests.el |  1 -
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/lisp/eshell/esh-io.el b/lisp/eshell/esh-io.el
index 3644c1a18b..2d25186de7 100644
--- a/lisp/eshell/esh-io.el
+++ b/lisp/eshell/esh-io.el
@@ -276,8 +276,19 @@ eshell-close-target
    ;; If we're redirecting to a process (via a pipe, or process
    ;; redirection), send it EOF so that it knows we're finished.
    ((eshell-processp target)
-    (if (eq (process-status target) 'run)
-	(process-send-eof target)))
+    ;; According to the POSIX standards, sending EOF causes all bytes
+    ;; waiting to be read to be sent to the process immediately.
+    ;; Thus, if there are any bytes waiting, we need to send EOF
+    ;; twice: once to flush the buffer, and a second time to cause the
+    ;; next read() to return a size of 0.  However, some platforms
+    ;; (e.g. Solaris) actually require a *third* EOF.  Since sending
+    ;; extra EOFs while the process is running shouldn't break
+    ;; anything, we'll just send the maximum we'd ever need.  See
+    ;; bug#56025 for further details.
+    (let ((i 0))
+      (while (and (<= (cl-incf i) 3)
+                  (eq (process-status target) 'run))
+        (process-send-eof target))))
 
    ;; A plain function redirection needs no additional arguments
    ;; passed.
diff --git a/test/lisp/eshell/em-extpipe-tests.el b/test/lisp/eshell/em-extpipe-tests.el
index 3b84d763ac..29f5dc0551 100644
--- a/test/lisp/eshell/em-extpipe-tests.el
+++ b/test/lisp/eshell/em-extpipe-tests.el
@@ -71,7 +71,6 @@ em-extpipe-tests--deftest
        (skip-unless shell-file-name)
        (skip-unless shell-command-switch)
        (skip-unless (executable-find shell-file-name))
-       (skip-unless (not (getenv "EMACS_EMBA_CI")))
        (let ((input ,input))
          (with-temp-eshell ,@body)))))
 
-- 
2.25.1