Fix SSH hangs #102
Merged
Fix SSH hangs #102
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Jun 23, 2025
dscho
force-pushed
the
fix-ssh-hangs-reloaded-g4w
branch
from
ec817d7 to
32200e2
Compare
Member
Author
Member
Author
|
/open pr The workflow run was started |
dscho
force-pushed
the
fix-ssh-hangs-reloaded-g4w
branch
from
32200e2 to
18ec25e
Compare
Member
Author
|
/open pr The workflow run was started |
When cbfaeba (Cygwin: pipe: Fix incorrect write length in raw_write(), 2024-11-06) fixed a bug where too-long writes could cause segmentation faults, it not only left out crucial details from the commit message, it also introduced a bug. This manifests e.g. in the symptom where cloning large repositories in Git for Windows via SSH "freeze" at random stages, as has been reported in git-for-windows/git#5688 and in git-for-windows/git#5682. The bug in question was to use `is_nonblocking ()` instead of `real_non_blocking_mode` in a newly introduced condition. If the commit message had filled in those details, it would most likely have been much easier to spot the bug before the patch was committed. Granted, the patch _sort of_ moved this `is_nonblocking ()` condition from another place, which means that the bug was present before, even if it did not have the same detrimental symptom of hanging a clone of a large repository via SSH. The _original_ bug was introduced in the equally under-explained 7ed9adb (Cygwin: pipe: Switch pipe mode to blocking mode by default, 2024-09-05). What is ironic is that this here patch is the latest (and hopefully last) commit in a _long_ chain of bug fixes that fix bugs introduced by preceding bug fixes: - 9e4d308 (Cygwin: pipe: Adopt FILE_SYNCHRONOUS_IO_NONALERT flag for read pipe., 2021-11-10) fixed a bug where Cygwin hung by mistake while piping output from one .NET program as input to another .NET program (potentially introduced by 3651990 (Cygwin: pipe: Avoid false EOF while reading output of C# programs., 2021-11-07), which was itself a bug fix). It introduced a bug that was fixed by... - fc691d0 (Cygwin: pipe: Make sure to set read pipe non-blocking for cygwin apps., 2024-03-11). Which introduced a bug that was purportedly fixed by... - 7ed9adb (Cygwin: pipe: Switch pipe mode to blocking mode by default, 2024-09-05). Which introduced a bug that was fixed by... - cbfaeba (Cygwin: pipe: Fix incorrect write length in raw_write(), 2024-11-06). Which introduced a bug that was fixed by... this here patch. There is not only the common thread here that each of these bug fixes introduced a new bug, but also the common thread of commit messages that leave the reader puzzled and the problem — as well as the solution — under-explained. It is quite likely that, had the time and care been spent to fully explain what is going on in the commit messages, this would quite likely have triggered a re-review of the diff by the author. In other words, writing a complete commit message could have increased the insight and understanding of the problem and thereby prevented this series of "fixes that introduce new bugs". To avoid making the same mistake once again, here is my attempt at a thorough explanation of what's going on, what is the problem, what solutions were considered, and why the "winner" was chosen. So let's start with an overview of the logic of the `fhandler_pipe_fifo::raw_write ()` method. This method implements the fundamental logic behind writing to any pipe in Cygwin, whether it be blocking or not blocking, and takes as input two variables, `ptr` and `len`, pointing to the data to be written. First, this method determines a couple of initial conditions (such as maybe waiting on a specific mutex, or determining whether the pipe had been closed already, or whether it is in blocking or in non-blocking mode). Then, this method determines how many bytes of data should be written in each attempt (assigned to the variable `chunk`). The amount of bytes already written is tracked as `nbytes`. While this value is smaller than `len`, the central loop of this method is executed. Counterintuitively, this loop is necessary even in non-blocking mode, to handle conditions like `STATUS_PENDING`. Inside this loop body, a variety of conditions are handled, including some that break out of that loop before `nbytes >= len`, e.g. when the pipe is closed on the other end. This loop body also contains special, complicated logic to handle the case where the pipe buffer is not large enough to handle the current `chunk`, trying to write even less bytes (this is guarded by the flag `short_write_once`). Unfortunately, the recent re-design of that logic decided to always set the pipe to blocking (cf. the diff of ed9adb356 (Cygwin: pipe: Switch pipe mode to blocking mode by default, 2024-09-05)). The rationale for that decision was the desire to simplify the code, which came at the expense of working well together with pipes that were created outside of Cygwin and that are set to non-blocking mode. To this end, the current implementation now _emulates_ non-blocking behavior while keeping the actual pipe in blocking mode. This requires knowing how much data can be written to the pipe without blocking at any given moment. Because by knowing that amount, even in blocking mode a write operation can be performed that does not block. This information is obtained via `NtQueryInformationFile(FilePipeLocalInformation)` and stored in the variable `avail`. However, if for some reason it cannot be determined how much space is available in the pipe, this non-blocking mode emulation would not work. Therefore the pipe is set to genuine non-blocking mode in such cases, and only in such cases. This minimizes the time window (but does not eliminate it completely!) during which Cygwin does not work well together with non-Cygwin processes via pipes. It also shows that the original desire to simplify the code can not be fulfilled using the current strategy, as there is now both code to emulate non-blocking mode as well as code that handles genuinely non-blocking mode. This information whether non-blocking mode is merely emulated or genuinely enabled, is tracked by the Boolean variable `real_non_blocking_mode`. At the beginning of the loop body, after determining how many bytes to write (stored in `len1`), the `NtWriteFile()` is called. Since the re-design to always use blocking mode (at least insofar possible), this `NtWriteFile()` call is guarded by a condition so that it is called only once in non-blocking mode. Now, the logic for that is quite complex. First, `len1` is clamped to `chunk` in blocking mode, otherwise to however many bytes are left to write. Then, an _inner_ `while` loop runs as long as `len1` is greater than 0. Inside that loop, `NtWriteFile()` is called _unless_ we'd try to write more bytes than are available in emulated non-blocking mode. It is the complex interplay between that clamping to `chunk` and the condition when `NtWriteFile()` is skipped that is at the heart of this bug. But let's first see what happens in the case described above, where the (Cygwin) SSH process gets stuck writing to the (non-Cygwin) Git process. In the instances I observed, the big `while (nbytes < len)` loop is started with the following initial conditions: `len` is 2097152, `chunk` and `avail` are 1 and `real_non_blocking_mode` is 0. When the outer loop is entered, `len1` is therefore also set to 2097152, and at the start of the inner loop the condition `len1 > avail` holds true. Therefore, the `NtWriteFile()` call is skipped already during the first iteration, we run into the code that tries a short write (setting `short_write_once` to 1), and ultimately fail and return from `raw_write()` with an error. Since not a single byte was written, the caller will try again at some stage, failing again, over and over, and we're in an infinite loop. The fix chosen in this here commit is to apply that clamping to `chunk` _also_ in the emulated non-blocking mode. This lets the `NtWriteFile()` call _not_ be skipped in non-emulated non-blocking mode, as was clearly the intention. An alternative patch that would have also addressed the symptom would be to partially revert this hunk of cbfaeba (Cygwin: pipe: Fix incorrect write length in raw_write(), 2024-11-06): chunk = len; - else if (is_nonblocking ()) - chunk = len = pipe_buf_size; else chunk = avail; The logic changed such that `len` would no longer be clamped to the same value as `chunk` in non-blocking mode. Reinstating that clamp in non-blocking mode would also work around the issue, but it would be a bit more heavy-handed than the chosen fix. Yet another potential fix that was suggested (in git-for-windows/git#5688 (comment)) would be to modify the condition when `NtWriteFile()` is skipped so as to force it to _not_ be skipped when attempting a "short write", i.e. when `short_write_once != 0`. This fix would have worked around the bug successfully, but is fixing the bug at the wrong layer (even if it hints at the fact that the `short_write_once` logic is ill-prepared for non-blocking mode, but that's a topic for another patch). Fixes: cbfaeba (Cygwin: pipe: Fix incorrect write length in raw_write(), 2024-11-06) Co-authored-by: Takashi Yano <[email protected]> Signed-off-by: Johannes Schindelin <[email protected]>
dscho
force-pushed
the
fix-ssh-hangs-reloaded-g4w
branch
from
18ec25e to
cb9b2d0
Compare
Member
Author
|
/open pr The workflow run was started |
github-actions bot
pushed a commit
to git-for-windows/build-extra
that referenced
this pull request
Jun 24, 2025
Cloning large repositories via SSH [frequently froze](git-for-windows/git#5688) as of Git for Windows v2.50.0, which [has been fixed](git-for-windows/msys2-runtime#102). Signed-off-by: gitforwindowshelper[bot] <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A report demonstrated that cloning large repositories via SSH frequently hangs:
and there it will stay.
This PR addresses that, and has been contributed to the
cygwin-patchesmailing list for review.