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

[Ken Brown <kbrown@cornell.edu>]

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