[PATCH v8] Cygwin: pipe: Switch pipe mode to blocking mode by default

[Takashi Yano]

On Wed, 25 Sep 2024 16:11:12 -0400
Ken Brown wrote:
> On 9/21/2024 5:15 PM, Takashi Yano wrote:
> > Previously, cygwin read pipe used non-blocking mode although non-
> > cygwin app uses blocking-mode by default. Despite this requirement,
> > if a cygwin app is executed from a non-cygwin app and the cygwin
> > app exits, read pipe remains on non-blocking mode because of the
> > commit fc691d0246b9. Due to this behaviour, the non-cygwin app
> > cannot read the pipe correctly after that. Similarly, if a non-
> > cygwin app is executed from a cygwin app and the non-cygwin app
> > exits, the read pipe mode remains on blocking mode although cygwin
> > read pipe should be non-blocking mode.
> > 
> > These bugs were provoked by pipe mode toggling between cygwin and
> > non-cygwin apps. To make management of pipe mode simpler, this
> > patch has re-designed the pipe implementation. In this new
> > implementation, both read and write pipe basically use only blocking
> > mode and the behaviour corresponding to the pipe mode is simulated
> > in raw_read() and raw_write(). Only when NtQueryInformationFile
> > (FilePipeLocalInformation) fails for some reasons, the raw_read()/
> > raw_write() cannot simulate non-blocking access. Therefore, the pipe
> > mode is temporarily changed to non-blocking mode.
> > 
> > Moreover, because the fact that NtSetInformationFile() in
> > set_pipe_non_blocking(true) fails with STATUS_PIPE_BUSY if the pipe
> > is not empty has been found, query handle is not necessary anymore.
> > This allows the implementation much simpler than before.
> > 
> > Addresses: https://github.com/git-for-windows/git/issues/5115
> > Fixes: fc691d0246b9 ("Cygwin: pipe: Make sure to set read pipe non-blocking for cygwin apps.");
> > Reported-by: isaacag, Johannes Schindelin <Johannes.Schindelin@gmx.de>
> > Reviewed-by: Corinna Vinschen <corinna@vinschen.de>, Ken Brown <kbrown@cornell.edu>
> > Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
> > ---
> >   winsup/cygwin/dtable.cc                 |   5 +-
> >   winsup/cygwin/fhandler/pipe.cc          | 657 ++++++++----------------
> >   winsup/cygwin/local_includes/fhandler.h |  44 +-
> >   winsup/cygwin/local_includes/sigproc.h  |   1 -
> >   winsup/cygwin/select.cc                 |  46 +-
> >   winsup/cygwin/sigproc.cc                |  10 -
> >   winsup/cygwin/spawn.cc                  |   4 -
> >   7 files changed, 252 insertions(+), 515 deletions(-)
> LGTM, but it's complicated enough that I could have missed something. 
> It will clearly need lots of testing.
> 
> One trivial suggestion: For clarity, you should probably add the 
> initialization of pipe_mtx to the fhandler_pipe_fifo constructor, 
> although I think it's initialized to NULL by default.  Also, it wouldn't 
> hurt to add a comment in fhandler.h that pipe_mtx is only used in the 
> pipe case, i.e., it remains NULL for fifos.

Thank you for reviewing and advice.
Do you mean testing enough before push? Or testing in the 3.6 branch
before release?

Anyway, I'd like to wait corinna before push.

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>