On Mon, 23 Jun 2025 14:09:49 +0200 (CEST) Johannes Schindelin wrote: > The recent changes in Cygwin's pipe logic are a gift that keeps on > giving. > > The recurring pattern is that bugs are fixed, but with many of these bug > fixes, new bugs are produced, and we are stuck in this seemingly endless > tunnel of fixing bugs caused by bug fixes. > > In cbfaeba4f7 (Cygwin: pipe: Fix incorrect write length in raw_write(), > 2024-11-06), for example, a segmentation fault was fixed. Which is good! > However, a bug was introduced, where e.g. Git for Windows' `git clone` > via SSH frequently hangs indefinitely for large-ish repositories, see > https://github.com/git-for-windows/git/issues/5688. > > That commit removed logic whereby in non-blocking mode, not only the > chunk size, but also the overall count of bytes to write (`len`) was > clamped to whatever is indicated as the `WriteQuotaAvailable`. Now only > `chunk` is clamped to that, but `len` is left at its original number. > However, the following `while` loop expects `len - chunk` (which is > assigned to `len1`) not to be positive in non-blocking mode. > > Let's reinstate that clamping logic and implicitly fix this SSH hang. > > Fixes: cbfaeba4f7 (Cygwin: pipe: Fix incorrect write length in raw_write(), > 2024-11-06) > Signed-off-by: Johannes Schindelin <[email protected]> > --- > Published-As: > https://github.com/dscho/msys2-runtime/releases/tag/fix-ssh-hangs-reloaded-v1 > Fetch-It-Via: git fetch https://github.com/dscho/msys2-runtime > fix-ssh-hangs-reloaded-v1 > > This commit message will like any love you can give it. For > example, I do not _quite_ understand why the `while` loop skips > large chunks of code unless `len1 > 0`, and what the exact idea > was behind having that `while` loop even for non-blocking mode. > Could anyone help me understand that `raw_write()` method? Is > there any good reason why the non-blocking mode runs into a > `while` loop? Is it supposed to be run only once in non-blocking > mode, is _that_ the big secret that allows the code to be shared > between blocking and non-blocking mode? If so, wouldn't it be much > better to refactor out that logic and then have non-blocking mode > take a short-cut, for clarity's sake and peace of readers' mind? > > What I am quite confident is that this works around the problems. > > I would have put more work into the commit message, if it weren't > for two counter-acting points: > > 1. This seems to be a pretty bad regression by which many Git for > Windows users are affected. So I do feel quite the pressure to > get a fix out to those users. > > 2. Despite my pleas, the commit messages in the pipe-related > changes keep having too many gaps, still leave way too much > unclear for me to make any sense of them, and I have to admit > that I do not want to be the only person in that space to put > in a large effort to write stellar commit messages. Therefore I > left this here commit message in a state I consider "good > enough", even if I am more than willing to improve it should > someone enlighten me as to the questions I raised above. > > winsup/cygwin/fhandler/pipe.cc | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/winsup/cygwin/fhandler/pipe.cc b/winsup/cygwin/fhandler/pipe.cc > index e35d523bbc..13af7f2ae1 100644 > --- a/winsup/cygwin/fhandler/pipe.cc > +++ b/winsup/cygwin/fhandler/pipe.cc > @@ -542,6 +542,8 @@ fhandler_pipe_fifo::raw_write (const void *ptr, size_t > len) > > if (len <= (size_t) avail) > chunk = len; > + else if (is_nonblocking ()) > + chunk = len = avail; > else > chunk = avail; > > > base-commit: 1186791e9f404644832023b8fa801227c2995ab7 > -- > 2.50.0.windows.1
Thanks for the patch. This indeed fixes the issue. However, I have another idea to fix the issue. Please see: https://github.com/git-for-windows/git/issues/5682#issuecomment-2996285140 -- Takashi Yano <[email protected]>
