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.

Sign up for GitHub

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

Jump to bottom

Socket.Windows: support ConnectAsync(SocketAsyncEventArgs) for UDP, and Unix sockets #33674

Merged
merged 5 commits into from
Apr 3, 2020
Merged

Socket.Windows: support ConnectAsync(SocketAsyncEventArgs) for UDP, and Unix sockets #33674

merged 5 commits into from
Apr 3, 2020

Conversation

tmds
Copy link
Member

@tmds tmds commented Mar 17, 2020

These are the changes from #787 that deal with making ConnectAsync(SocketAsyncEventArgs) work for non-stream protocols (like UDP) on Windows.

cc @stephentoub @antonfirsov @dotnet/ncl

antonfirsov
antonfirsov approved these changes Mar 17, 2020
View reviewed changes
Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@davidsh davidsh added this to the 5.0 milestone Mar 17, 2020
@davidsh
Copy link
Contributor

davidsh commented Mar 17, 2020

We'll want to run Outerloop tests before this can be merged.

scalablecory
scalablecory approved these changes Mar 17, 2020
View reviewed changes
Copy link
Contributor

@scalablecory scalablecory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small question/nit, otherwise looks good.

@antonfirsov
Copy link
Member

antonfirsov commented Mar 19, 2020 •
edited
Loading

Looks like there's no need to check _rightEndpoint: both ConnectAsync and BeginConnect will ensure that the sockets are bound in WildcardBindForConnectIfNecessary anyways, so we can get rid of this obscure logic.

@tmds with this change all tests are passing. It also includes an additional test for all connect variants to improve coverage.

@tmds tmds changed the title Socket.Windows: support ConnectAsync(SocketAsyncEventArgs) for non-stream protocols Socket.Windows: support ConnectAsync(SocketAsyncEventArgs) for UDP, and Unix sockets Mar 21, 2020
@antonfirsov
Copy link
Member

/azp run runtime-libraries outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@antonfirsov
Copy link
Member

antonfirsov commented Mar 21, 2020 •
edited
Loading

I'm guilty of not testing my proposal locally on Linux, and it looks like we have a bunch of related test failures, mostly specific to connect with IPv6 .

I guess we are still missing something about that obscure _rightEndPoint != null || remoteEP.GetType() == typeof(IPEndPoint) logic.

tmds
tmds commented Mar 21, 2020
View reviewed changes
src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEventArgs.Windows.cs
@@ -1160,6 +1168,13 @@ private unsafe SocketError FinishOperationConnect()
{
try
{
if (_currentSocket!.SocketType != SocketType.Stream)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@antonfirsov @scalablecory also give this condition some thought. Personally, I'm fine if CI passes. You may have a better understanding of what SO_UPDATE_CONNECT_CONTEXT does to decide if this is the most appropriate check.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should match the logic for CanUseConnectEx. We need to call SO_UPDATE_CONNECT_CONTEXT if ConnectEx is used.

Copy link
Member Author

@tmds tmds Mar 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking the same thing, though it seems the stream unix socket, which doesn't support ConnectEx, doesn't give an error when we set this socket option.
We added the check because UDP socket gave an error.

@jaredpar jaredpar mentioned this pull request Mar 21, 2020
Closed
@tmds
Copy link
Member Author

tmds commented Mar 21, 2020

@antonfirsov , you can push commits to this branch.

@antonfirsov antonfirsov added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 21, 2020
@ericstj ericstj mentioned this pull request Mar 23, 2020
Closed
@tmds
Copy link
Member Author

tmds commented Mar 31, 2020

@antonfirsov I noticed the failures are not Windows-only, I will investigate further.

@antonfirsov
Copy link
Member

@tmds yes, they are mostly specific to Linux and IPv6 as far as I remember.

@tmds
Copy link
Member Author

tmds commented Apr 2, 2020

@scalablecory @antonfirsov CanUseConnectEx now has proper implementation. I included comments to explain what it does. Tests pass in CI. Can you take another look?

@scalablecory
Copy link
Contributor

Thank you! Will take a look shortly.

@antonfirsov
Copy link
Member

/azp run runtime-libraries outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

antonfirsov
antonfirsov reviewed Apr 3, 2020
View reviewed changes
src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs

return (_socketType == SocketType.Stream) &&
(_rightEndPoint != null || remoteEP.GetType() == typeof(IPEndPoint)) &&
(remoteEP.AddressFamily != AddressFamily.Unix);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if @stephentoub's #33674 (comment) also applies to AddressFamily ? Wouldn't it be more future proof to use the condition (remoteEP.AddressFamily == AddressFamily.InterNetwork || remoteEP.AddressFamily == AddressFamily.InterNetworkV6) instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no preference for remoteEP.GetType() == typeof(IPEndPoint) vs (remoteEP.AddressFamily == AddressFamily.InterNetwork || remoteEP.AddressFamily == AddressFamily.InterNetworkV6), so I kept the check as it was.

antonfirsov
antonfirsov approved these changes Apr 3, 2020
View reviewed changes
Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, and all test failures are unrelated. If someone else can confirm, I think we are safe to merge.

@antonfirsov antonfirsov removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Apr 3, 2020
scalablecory
scalablecory approved these changes Apr 3, 2020
View reviewed changes
Copy link
Contributor

@scalablecory scalablecory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me.

@scalablecory scalablecory merged commit b755ba9 into dotnet:master Apr 3, 2020
@tmds tmds mentioned this pull request Apr 6, 2020
Merged
@wfurt wfurt mentioned this pull request Jul 14, 2020
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub left review comments

@antonfirsov antonfirsov antonfirsov approved these changes

@scalablecory scalablecory scalablecory approved these changes

Assignees
No one assigned
Labels
area-System.Net.Sockets
Projects
None yet
Milestone
5.0.0
Development

Successfully merging this pull request may close these issues.
6 participants
@tmds @davidsh @antonfirsov @scalablecory @stephentoub @Dotnet-GitSync-Bot