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

From: Jim Porter <[email-masked]>
Date: Mon, 27 Jun 2022 12:18:05 -0700

On 6/27/2022 6:25 AM, Ken Brown wrote:
> Thanks to all of you for working on this while I was gone.  
> Unfortunately, the problem is still present on Cygwin.  In my haste to 
> get away, I neglected to mention that there is apparently a timing issue 
> in Eshell on Cygwin, so that even three EOFs do not always suffice to 
> kill the process.
> 
> My test case is to run
> 
>    echo bar | sh -c rev
> 
> in Eshell.  For reasons I don't understand, EOF almost always has to be 
> sent more than 3 times times before the "sh" process dies.  The maximum 
> I've observed is 93.  Inserting "(sit-for 0.01)" after each EOF 
> eliminates the need for extra EOFs; this is why I referred to the 
> problem as a timing issue.
> 
> I propose the following workaround:
> 
> --- a/lisp/eshell/esh-io.el
> +++ b/lisp/eshell/esh-io.el
> @@ -284,10 +284,16 @@ eshell-close-target
>       ;; end-of-file to the reading process.  However, some platforms
>       ;; (e.g. Solaris) actually require sending 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)
> +    ;; anything, we'll send up to three on all platforms.
> +
> +    ;; There's an extra wrinkle on Cygwin where, apparently due to an
> +    ;; unknown timing issue, it sometimes takes more than three EOFs
> +    ;; to kill the process.  (This only happens in Eshell, not in an
> +    ;; ordinary Cygwin shell.)  We work around this problem by sending
> +    ;; up to 1000 EOFs on Cygwin.  See bug#56025 for further details.
> +    (let ((i 0)
> +          (n (if (eq system-type 'cygwin) 1000 3)))
> +      (while (and (<= (cl-incf i) n)
>                     (eq (process-status target) 'run))
>           (process-send-eof target))))

I'd be very hesitant to do this, since as you mention above, this seems 
like a timing issue, and it's entirely possible that there are other, 
more widespread issues on Cygwin here. We'd also want to check the 
system that the process is actually running on; otherwise, remoting into 
a Cygwin system (via Tramp) would still exhibit the problem. I'll see if 
I can get a Cygwin environment up to test things out in the next week-ish.

If there's no other way that we can come up with here, I'd lean towards 
a defcustom so that users can tweak this if needed.