Hi Takashi,
On Wed, 25 Jun 2025, Takashi Yano wrote:
> pipe_data_available() is called in the raw_write() and if the pipe
> is real_non_blocking_mode in raw_write(), the pipe mode can be
> accidentally reverted to blocking mode in some cases by calling
> pipe_data_available().
Okay, let's unpack this a bit, because in the current form, the commit
message is inadequate.
So `pipe_data_available()` is called in `raw_write()`. But when?
Looking at the code, it seems that it is only called in the code that
works around Windows-specific behavior that is different from what Linux
does, by reducing the number of bytes to write after a failed attempt.
The most crucial point (which _must_ be mentioned in the commit message!)
is that `pipe_data_available()` — despite its name suggesting that it is
purely an accessor — is a _mutator_! That is, under certain circumstances
(when `NtQueryInformationFile()` failed to figure out the pipe's buffer
size) it temporarily flips the mode to non-blocking. According to the code
comment in `select.cc`, which should at least partially be recapitulated
in this patch's commit message, this mode flipping is done solely to find
out whether a read is pending).
Never mind that it tries to flip the mode whether the pipe is in blocking
mode or not (and 7ed9adb356 (Cygwin: pipe: Switch pipe mode to blocking
mode by default, 2024-09-05) keeps mum about the reasons, too) it always
flips the mode "back" to blocking.
> With this patch, pipe_data_available() reffers current real blocking
> mode, and restores the pipe mode to the current state.
This fails to answer the obvious question whether the method of mode
flipping works in either direction to find out whether a read is pending
on the other side. In fact, it is not even clear to me whether you
verified that this is so, or whether you found documentation saying that
it behaves this way.
And since this purportedly fixes a bug, the question begs itself whether
there should not be a new test case in the test suite to prevent
regressions.
> Fixes: 7ed9adb356df ("Cygwin: pipe: Switch pipe mode to blocking mode by
> default")
> Reported-by: Andrew Ng (@github)
No, Andrew's handle is not `@github`. And the actual reference is missing
and needs to be added:
https://github.com/git-for-windows/git/issues/5682#issuecomment-2997428207
> Reviewed-by:
> Signed-off-by: Takashi Yano <[email protected]>
> ---
> winsup/cygwin/fhandler/pipe.cc | 2 --
> winsup/cygwin/local_includes/fhandler.h | 3 +++
> winsup/cygwin/select.cc | 11 ++++++-----
> 3 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/winsup/cygwin/fhandler/pipe.cc b/winsup/cygwin/fhandler/pipe.cc
> index 4f23bd38c..ea0a8d807 100644
> --- a/winsup/cygwin/fhandler/pipe.cc
> +++ b/winsup/cygwin/fhandler/pipe.cc
> @@ -326,7 +326,6 @@ fhandler_pipe::raw_read (void *ptr, size_t& len)
> ULONG_PTR nbytes_now = 0;
> ULONG len1 = (ULONG) (len - nbytes);
> DWORD select_sem_timeout = 0;
> - bool real_non_blocking_mode = false;
I'll go ahead and take this as a belated answer to my question why
`real_non_blocking_mode` isn't a class attribute. It now is!
>
> FILE_PIPE_LOCAL_INFORMATION fpli;
> status = NtQueryInformationFile (get_handle (), &io,
> @@ -453,7 +452,6 @@ fhandler_pipe_fifo::raw_write (const void *ptr, size_t
> len)
> return 0;
>
> ssize_t avail = pipe_buf_size;
> - bool real_non_blocking_mode = false;
>
> /* Workaround for native ninja. Native ninja creates pipe with size == 0,
> and starts cygwin process with that pipe. */
> diff --git a/winsup/cygwin/local_includes/fhandler.h
> b/winsup/cygwin/local_includes/fhandler.h
> index 3d9bc9fa5..04e2ca4c3 100644
> --- a/winsup/cygwin/local_includes/fhandler.h
> +++ b/winsup/cygwin/local_includes/fhandler.h
> @@ -1203,6 +1203,7 @@ class fhandler_pipe_fifo: public fhandler_base
> protected:
> size_t pipe_buf_size;
> HANDLE pipe_mtx; /* Used only in the pipe case */
> + bool real_non_blocking_mode; /* Used only in the pipe case */
> virtual void release_select_sem (const char *) {};
>
> IMPLEMENT_STATUS_FLAG (bool, isclosed)
> @@ -1212,6 +1213,8 @@ class fhandler_pipe_fifo: public fhandler_base
>
> virtual bool reader_closed () { return false; };
> ssize_t raw_write (const void *ptr, size_t len);
> +
> + friend ssize_t pipe_data_available (int, fhandler_base *, HANDLE, int);
> };
>
> class fhandler_pipe: public fhandler_pipe_fifo
> diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc
> index bb141b065..32c73fd0c 100644
> --- a/winsup/cygwin/select.cc
> +++ b/winsup/cygwin/select.cc
> @@ -644,22 +644,23 @@ pipe_data_available (int fd, fhandler_base *fh, HANDLE
> h, int mode)
> reader is currently trying to read the pipe and it is pending.
> In the latter case, the fact that the reader cannot read the data
> immediately means that the pipe is empty. In the former case,
> - NtSetInformationFile() in set_pipe_non_blocking(true) will fail
> - with STATUS_PIPE_BUSY, while it succeeds in the latter case.
> + NtSetInformationFile() in set_pipe_non_blocking(!orig_mode) will
> + fail with STATUS_PIPE_BUSY, while it succeeds in the latter case.
> Therefore, we can distinguish these cases by calling set_pipe_non_
> blocking(true). If it returns success, the pipe is empty, so we
> return the pipe buffer size. Otherwise, we return 0. */
> if (fh->get_device () == FH_PIPEW && fpli.WriteQuotaAvailable == 0)
> {
> + bool orig_mode = ((fhandler_pipe *) fh)->real_non_blocking_mode;
> NTSTATUS status =
> - ((fhandler_pipe *) fh)->set_pipe_non_blocking (true);
> + ((fhandler_pipe *) fh)->set_pipe_non_blocking (!orig_mode);
> if (status == STATUS_PIPE_BUSY)
> return 0; /* Full */
> else if (!NT_SUCCESS (status))
> /* We cannot know actual write pipe space. */
> return 1;
> - /* Restore pipe mode to blocking mode */
> - ((fhandler_pipe *) fh)->set_pipe_non_blocking (false);
> + /* Restore pipe mode to original blocking mode */
> + ((fhandler_pipe *) fh)->set_pipe_non_blocking (orig_mode);
> /* Empty */
> fpli.WriteQuotaAvailable = fpli.InboundQuota;
> }
Okay, this diff makes sense to me, with the information I cobbled together
from doing half an hour of research. It should not take half an hour for
every reader to be able to understand the reasoning, though.
Please rework the commit message.
Ciao,
Johannes
