Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix: Support failed async connects on windows. #930

Closed
wants to merge 3 commits into from

Conversation

kristjanvalur
Copy link
Contributor

On Windows, a failed non-blocking connect() must be resolved by getting the socket error using getsockopt(). In fact, this
is the recommended way on posix too, not to repeat a call to connect(), but call getsockopt until it succeeds or fails.
This PR adds error translation to getsockopt(), adds an edge case for winsock (WSAEINVAL) and enables getsockopt() processing when connect() indicates that it is prudent.
This allows async connects to fail on windows with the proper error code.

@kristjanvalur
Copy link
Contributor Author

kristjanvalur commented Mar 29, 2021

Please note that the correct way to do a non-blocking connect() call is make a single call to connect(), then subsequently do only getsockopt() to query the socket status. Otherwise, connect() would have been endowed with a EWOULDBLOCK error like the other calls designed to be non-blocking. This is the reason for the multiple platform-dependent way to check for a failing non-blocking connect, there wasn't really a specification for how a subsequent call should behave.
It would be cleaner to have a separate function to initiate a connect and another to check it subsequently, but I didn't want to make the patch overly complicated and risky.

@kristjanvalur kristjanvalur changed the title Support failed async connects on windows. Bugfix: Support failed async connects on windows. Apr 8, 2021
@kristjanvalur
Copy link
Contributor Author

It would be awesome to get some traction on this, it has been half a year now

@kristjanvalur
Copy link
Contributor Author

Pinging this.

@kristjanvalur
Copy link
Contributor Author

The skipped tests causing failure were due to detect_debug_sleep(c) in test.c failing, nothing to do with this pr.

@kristjanvalur
Copy link
Contributor Author

@michael-grunder , could we get this merged?

@michael-grunder
Copy link
Collaborator

I cherry-picked your commits to keep the git history clean.

@kristjanvalur
Copy link
Contributor Author

No problem. You can also ask me to rebase these PRs before merging, they've been lingering here for a long time and had Main merged in quite often.

@kristjanvalur kristjanvalur deleted the pr1 branch June 27, 2022 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants