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

[Ken Brown]

On 9/26/2024 8:09 AM, Takashi Yano wrote:
> 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?

I was thinking mainly about testing before pushing.

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

Ken