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

multi: implement wait using winsock #5397

Closed
wants to merge 1 commit into from
Closed

Conversation

@rcombs
Copy link
Contributor

rcombs commented May 14, 2020

This avoids using a pair of TCP ports to provide wakeup functionality for every multi instance on Windows, where socketpair() is emulated using a TCP socket on loopback.

I'm hoping this can get some review and testing from people more familiar with Winsock than I am.

@rcombs rcombs force-pushed the rcombs:winsock-wakeup branch from 19ca632 to 392865e May 14, 2020
@mback2k mback2k self-requested a review May 14, 2020
@rcombs rcombs force-pushed the rcombs:winsock-wakeup branch from 392865e to 52dbab3 May 14, 2020
lib/multi.c Outdated Show resolved Hide resolved
lib/multi.c Outdated Show resolved Hide resolved
lib/multi.c Outdated Show resolved Hide resolved
@mback2k mback2k self-assigned this May 14, 2020
@mback2k mback2k added the Windows label May 14, 2020
lib/multi.c Outdated Show resolved Hide resolved
lib/multi.c Outdated Show resolved Hide resolved
@mback2k mback2k removed their assignment May 19, 2020
@rcombs rcombs force-pushed the rcombs:winsock-wakeup branch from 52dbab3 to a21d275 May 27, 2020
Copy link
Member

mback2k left a comment

This is still failing test 1564 on Azure Pipelines CI. Please make sure that test works, thanks.

@ngg
Copy link
Contributor

ngg commented Jun 6, 2020

WSACreateEvent and other related functions are only available from Windows 8.1.
Please make sure that compatibility with older Windows versions are kept if you decide on merging this. (For example by checking windows target version at compile time or dynamically at runtime and falling back to the socketpair emulation solution if it's not available).

@rcombs
Copy link
Contributor Author

rcombs commented Jun 6, 2020

Sorry, what? The docs indicate that these functions were available at least as far back as Vista, and as far as I can tell they were also available way back in NT4.

@mback2k
Copy link
Member

mback2k commented Jun 6, 2020

The functions are indeed available since Windows 95 / NT4. The documentation only mentions Windows 8.1 and Windows Vista due to the introduction of Universal Windows Platform apps and the end of support for Windows 7. Microsoft has removed the information about older Windows versions from their updated documentation, which is very unfortunate.

@ngg
Copy link
Contributor

ngg commented Jun 7, 2020

Sry, I was just confused by MSDN docs then, it used to tell the minimum version exactly.

lib/multi.c Outdated Show resolved Hide resolved
lib/multi.c Outdated Show resolved Hide resolved
@rcombs rcombs force-pushed the rcombs:winsock-wakeup branch 2 times, most recently from 0fa9ac1 to 3e46eda Jun 15, 2020
@rcombs rcombs requested a review from mback2k Jun 16, 2020
@rcombs
Copy link
Contributor Author

rcombs commented Jun 16, 2020

Restructured to just reuse 1 event object, which also cuts down on allocations quite a lot. Tests are now passing.

lib/multi.c Show resolved Hide resolved
lib/multi.c Show resolved Hide resolved
lib/multi.c Outdated Show resolved Hide resolved
lib/multi.c Show resolved Hide resolved
lib/multi.c Outdated Show resolved Hide resolved
@mback2k
Copy link
Member

mback2k commented Jun 16, 2020

I think this PR is ready to go soon once my final remarks have been addressed. Thanks @rcombs!

@bagder do you think this can land during the feature freeze as this fixes resource (socket) exhaustion issues on Windows?

This avoids using a pair of TCP ports to provide wakeup functionality
for every multi instance on Windows, where socketpair() is emulated
using a TCP socket on loopback.
@rcombs rcombs force-pushed the rcombs:winsock-wakeup branch from 3e46eda to 06ec0e0 Jun 16, 2020
@rcombs rcombs requested a review from mback2k Jun 16, 2020
@mback2k mback2k closed this in 8bc25c5 Jun 17, 2020
@mback2k
Copy link
Member

mback2k commented Jun 17, 2020

Thanks a lot! I merged this since I consider this a bugfix which should be part of this release.

bagder added a commit that referenced this pull request Jun 30, 2020
This reverts commit 8bc25c5.

That commit (from #5397) introduced a regression in 7.71.0.

Reported-by: tmkk on github
Fixes #5631
@bagder
Copy link
Member

bagder commented Jun 30, 2020

This change seems to have caused a regression. See #5631. I'm about to revert this in #5632. The test case in 5631 should be a good one to add and make sure works before we attempt to land this take again.

bagder added a commit that referenced this pull request Jun 30, 2020
This reverts commit 8bc25c5.

That commit (from #5397) introduced a regression in 7.71.0.

Reported-by: tmkk on github
Fixes #5631
Closes #5632
@mback2k
Copy link
Member

mback2k commented Jun 30, 2020

@rcombs would you mind taking a look at #5631?

rcombs added a commit to rcombs/curl that referenced this pull request Jun 30, 2020
This avoids using a pair of TCP ports to provide wakeup functionality
for every multi instance on Windows, where socketpair() is emulated
using a TCP socket on loopback which could in turn lead to socket
resource exhaustion.

A previous version of this patch failed to account for how in Winsock,
FD_WRITE is set only once when writing becomes possible and not again
until after a send has failed due to the buffer filling. This contrasts
to how FD_READ and FD_OOB continue to be set until the conditions they
refer to no longer apply. This meant that if a user wrote some data to
a socket, but not enough data to completely fill its send buffer, then
waited on that socket to become writable, we'd erroneously stall until
their configured timeout rather than returning immediately.

This version of the patch addresses that issue by checking each socket
we're waiting on to become writable with select() before the wait, and
zeroing the timeout if it's already writable.

Closes curl#5397
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.