Update Cygwin ICMP service thread for asynchronous pipes #495
Conversation
This file contains 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
|
Actually, upon further investigation, WaitForSingleObject on the named pipe handle doesn't accomplish what I thought it would. Do not merge this PR yet - I will update with a better solution. |
Recent versions of Cygwin implement pipe() using Windows' named pipes, and put the read end of the pipe in FILE_PIPE_COMPLETE_OPERATION mode, which doesn't allow overlapped I/O operations. For the relevant commit in the Cygwin repository, see 9e4d308cd592fe383dec58ea6523c1b436888ef8 The solution here is to maintain a Windows event object which is set only when any ICMP requests are pending. We can do an alertable wait on that event object, which will allow us to complete ICMP requests. Thanks to Adam Schultz for research into this issue and a first attempt at a fix.
matt-kimball
force-pushed
the
cygwin-async-pipe
branch
from
bd0f147
to
dffd81b
Compare
|
PR updated. Unfortunately, the solution was slightly more complicated and uses some new Windows synchronization primitives, but now actually works to block in the alertable wait. I have tested this with newer versions of Cygwin, and it works as expected, but I also believe it will work for the older versions of Cygwin, and is less fragile since it no longer retrieves the Windows |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Recent versions of Cygwin implement pipe() using Windows' named pipes, and put the read end of the pipe in FILE_PIPE_COMPLETE_OPERATION mode, which doesn't allow overlapped I/O operations.
For the relevant commit in the Cygwin repository, see 9e4d308cd592fe383dec58ea6523c1b436888ef8
Happily, this mode is approximately equivalent to Unix's non-blocking read mode, so we can avoid overlapped I/O entirely, and instead wait on the Windows handle for the read end of the pipe when we need an alertable wait to receive ICMP probe completions.
Thanks to Adam Schultz for research into this issue and a first attempt at a fix.
This fixes issue #465