Cygwin: pipe: Restore blocking mode of read pipe on close() #72
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
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 fc691d0. Due to this behaviour, the non-cygwin app cannot read the pipe correctly after that. With this patch, the blocking mode of the read pipe is stored into was_blocking_read_pipe on set_pipe_non_blocking() when the cygwin app starts and restored on close(). Addresses: git-for-windows/git#5115 Fixes: fc691d0 ("Cygwin: pipe: Make sure to set read pipe non-blocking for cygwin apps."); Reported-by: isaacag, Johannes Schindelin <[email protected]> Reported-at: git-for-windows/git#5115 Signed-off-by: Takashi Yano <[email protected]> Signed-off-by: Johannes Schindelin <[email protected]>
Merged
Member
Author
|
There is now already a v4.2 (I am a bit confused what the .2 stands for, I don't find any clear explanation for it) but it has issues. My hand is forced quite a bit by Git v2.46.1 having been released yesterday and the corresponding Git for Windows version seems the only realistic chance to fix this issue for Windows 7 and Windows 8 users (because Git for Windows v2.47 will drop support for those Windows versions). So I'll just go ahead and merge this, and then prepare a corresponding MSYS2-packages PR and deploy the msys2-runtime package. |
dscho
added a commit
to dscho/MSYS2-packages
that referenced
this pull request
Sep 15, 2024
The Cygwin runtime (and for that reason, the MSYS2 runtime, too), change pipes from blocking to non-blocking, which is a problem when the pipe is actually opened by something like Git when it asks a credential helper for information. Git, not expecting the pipe mode to be changed behind its back, will then error out with "fatal: read error: Invalid argument". To address this, this patch was proposed in the Cygwin project as https://inbox.sourceware.org/cygwin-patches/[email protected]/ Ideally, Cygwin would not even _try_ to fiddle with the pipe mode, therefore this patch was not applied as-is. At time of writing, there is no consensus on any replacement patch, but I have to prepare _something_ for the already-late Git for Windows v2.46.1 (which is likely the last Git for Windows version to support Windows 7 and Windows 8, hence the urgency). And this patch at least improves the situation. This patch will be dropped when Git for Windows will upgrade to MSYS2 runtime v3.5, and hopefully consensus will have been reached about a better fix by that time. This commit corresponds to git-for-windows/msys2-runtime#72 which fixes git-for-windows/git#5115. Signed-off-by: Johannes Schindelin <[email protected]>
dscho
added a commit
to dscho/MSYS2-packages
that referenced
this pull request
Sep 15, 2024
The Cygwin runtime (and for that reason, the MSYS2 runtime, too), change pipes from blocking to non-blocking, which is a problem when the pipe is actually opened by something like Git when it asks a credential helper for information. Git, not expecting the pipe mode to be changed behind its back, will then error out with "fatal: read error: Invalid argument". To address this, this patch was proposed in the Cygwin project as https://inbox.sourceware.org/cygwin-patches/[email protected]/ Ideally, Cygwin would not even _try_ to fiddle with the pipe mode, therefore this patch was not applied as-is. At time of writing, there is no consensus on any replacement patch, but I have to prepare _something_ for the already-late Git for Windows v2.46.1 (which is likely the last Git for Windows version to support Windows 7 and Windows 8, hence the urgency). And this patch at least improves the situation. This patch will be dropped when Git for Windows will upgrade to MSYS2 runtime v3.5, and hopefully consensus will have been reached about a better fix by that time. This commit corresponds to git-for-windows/msys2-runtime#72 which fixes git-for-windows/git#5115. Signed-off-by: Johannes Schindelin <[email protected]>
dscho
added a commit
to dscho/msys2-runtime
that referenced
this pull request
Sep 18, 2024
…ng-mode Cygwin: pipe: Restore blocking mode of read pipe on close()
dscho
added a commit
to dscho/msys2-runtime
that referenced
this pull request
Sep 25, 2024
This includes the pull request git-for-windows#72 from dscho/restore-pipes-blocking-mode, and adds two more backported patches. Signed-off-by: Johannes Schindelin <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
As of cygwin/cygwin@fc691d0246b9, which was backported to the
cygwin-3_5-branchas cygwin/cygwin@55431b4, which in turn was backported into Git for Windows as a4d92d6, read pipes are essentially always marked as non-blocking.This strategy fails to take into account that pipes that were originally marked as blocking when they were created outside of MSYS/Cygwin are typically expected to remain in blocking mode by the processes that created them.
This leads to problems, e.g. the unfortunate symptom where using VSCode's built-in
askpasshelper, Git operations fail with this rather cryptic error message:Work around this not by leaving the blocking/non-blocking mode alone, but instead by still forcing the read pipe to non-blocking mode as long as Cygwin reads it yet ensuring that the mode is restored when the MSYS/Cygwin end of the pipe is closed. This logic would not appear to be thread-safe in the hope that it does not matter in practice.
This addresses git-for-windows/git#5115
/cc @tyan0