[PATCH v8] Cygwin: pipe: Switch pipe mode to blocking mode by default
Ken Brown
kbrown@cornell.edu
Wed Sep 25 20:11:12 GMT 2024
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.
Ken
More information about the Cygwin-patches
mailing list