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

[Ken Brown]

On 6/19/2022 12:02 PM, Ken Brown wrote:
> On 6/18/2022 6:00 PM, Jim Porter wrote:
> > On 6/18/2022 1:51 PM, Ken Brown wrote:
> > > On 6/18/2022 3:02 PM, Jim Porter wrote:
> > > > On 6/18/2022 10:52 AM, Ken Brown wrote:
> > > > > No, I'm seeing the same results on Emacs 28.  On both Emacs 28 and Emacs 
> > > > > 29, rev is apparently not seeing EOF unless echo outputs a newline, so rev 
> > > > > keeps waiting for input.
> > > >
> > > > Ah ha! Thanks for debugging this. The minimal fix then would be to change 
> > > > the command in em-extpipe-test-2 to either of these:
> > > >
> > > >    echo -N "bar" | rev *>temp
> > >
> > > This doesn't work.  It still hangs when run interactively...
> >
> > Just to confirm, the above command hangs, but the following works, correct?
> >
> >    echo -N "bar" | rev
>
> Correct.
>
> > > >    *echo "bar" | rev *>temp
> > >
> > > This works interactively...
> >
> > All this makes me think that we could be dealing with a race condition in how 
> > Eshell pipes I/O around. Maybe there's a timing issue in `eshell-close-target' 
> > where we end up not sending EOF to the "rev" (or "sh") process?
>
> I think I've just discovered an anomaly in "rev" on Cygwin that could partially 
> explain what I'm seeing.  I'll investigate that before proceeding further.

OK, I think I've got it sorted out now. The anomaly I referred to above is 
actually an anomaly in the stdio routines, not in "rev". It's discussed in item 
2 below. There are two issues.

1. I think there's a bug in eshell-close-target, in which it's assumed that 
sending C-d indicates end-of-file. This is only true if there's no input waiting 
to be read.  [In an interactive situation, this means we're at the beginning of 
a line.]  Otherwise, it takes a second C-d to indicate EOF.  So one C-d should 
suffice in the "echo -N bar" situation, but two are needed after "echo bar".

This bug probably went unnoticed because eshell-close-target was called twice in 
the case we were discussing, so process-send-eof was called twice.

2. On Cygwin and some other platforms, including Solaris 11.4 I think, it 
actually takes a third C-d, for reasons explained in the email thread starting 
at https://cygwin.com/pipermail/cygwin/2022-June/251672.html.  We're probably 
going to change this on Cygwin, but that still leaves other platforms.

The following patch resolves both issues:

diff --git a/lisp/eshell/esh-io.el b/lisp/eshell/esh-io.el
index 3644c1a18b..1c4131cb07 100644
--- a/lisp/eshell/esh-io.el
+++ b/lisp/eshell/esh-io.el
@@ -276,8 +276,8 @@ 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)))
+    (while (eq (process-status target) 'run)
+      (process-send-eof target)))

    ;; A plain function redirection needs no additional arguments
    ;; passed.

I'm about to go AFK for a few days.  If the eshell people agree that something 
like this patch should be installed, please go ahead.  I think it would then be 
worth re-enabling the extpipe tests on EMBA to see if the problem is fixed there 
too.

Ken