Socket.Windows: support ConnectAsync(SocketAsyncEventArgs) for UDP, and Unix sockets by tmds · Pull Request #33674 · dotnet/runtime

tmds · 2020-03-17T15:49:54Z

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

…ream protocols

antonfirsov

LGTM

davidsh · 2020-03-17T17:13:58Z

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

src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs

scalablecory

small question/nit, otherwise looks good.

antonfirsov · 2020-03-19T23:13:39Z

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.

antonfirsov · 2020-03-21T15:12:39Z

/azp run runtime-libraries outerloop

azure-pipelines · 2020-03-21T15:12:54Z

Azure Pipelines successfully started running 1 pipeline(s).

antonfirsov · 2020-03-21T15:25:04Z

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 · 2020-03-21T16:31:23Z

src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEventArgs.Windows.cs

        {
            try
            {
+                if (_currentSocket!.SocketType != SocketType.Stream)


@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.

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

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.

tmds · 2020-03-21T17:41:22Z

@antonfirsov , you can push commits to this branch.

tmds · 2020-03-31T15:30:20Z

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

antonfirsov · 2020-03-31T18:39:50Z

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

tmds · 2020-04-02T16:55:26Z

@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 · 2020-04-02T19:15:08Z

Thank you! Will take a look shortly.

antonfirsov · 2020-04-02T22:51:39Z

/azp run runtime-libraries outerloop

azure-pipelines · 2020-04-02T22:51:52Z

Azure Pipelines successfully started running 1 pipeline(s).

antonfirsov · 2020-04-03T12:11:38Z

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);


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?

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

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

scalablecory

looks good to me.

Socket.Windows: support ConnectAsync(SocketAsyncEventArgs) for non-st…

08ce33a

…ream protocols

Dotnet-GitSync-Bot added the area-System.Net.Sockets label Mar 17, 2020

antonfirsov approved these changes Mar 17, 2020

View reviewed changes

davidsh added this to the 5.0 milestone Mar 17, 2020

scalablecory reviewed Mar 17, 2020

View reviewed changes

src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs Outdated Show resolved Hide resolved

scalablecory approved these changes Mar 17, 2020

View reviewed changes

Check CanUseConnectEx later

1796b80

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

Update CanUseConnectEx for handling Unix socket addresses

dce3be8

tmds commented Mar 21, 2020

View reviewed changes

jaredpar mentioned this pull request Mar 21, 2020

Several tests fail on mono with mono_threads_pthread_kill: pthread_kill failed with error 11 #32377

Closed

antonfirsov added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 21, 2020

ericstj mentioned this pull request Mar 23, 2020

System.Text.Json.Serialization.Tests.ConstructorTests_StringTValue.OtherPropertiesAreSet fails with nullref #33928

Closed

tmds added 2 commits April 1, 2020 21:34

Improve CanUseConnectEx

abf67a1

Skip Connect_Dns_Success when localhost doesn't resolve to ::1

0fc45ba

antonfirsov reviewed Apr 3, 2020

View reviewed changes

antonfirsov approved these changes Apr 3, 2020

View reviewed changes

antonfirsov removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Apr 3, 2020

scalablecory approved these changes Apr 3, 2020

View reviewed changes

scalablecory merged commit b755ba9 into dotnet:master Apr 3, 2020

tmds mentioned this pull request Apr 6, 2020

Refactor Task Socket.ConnectAsync methods to use AwaitableSocketAsyncEventArgs #787

Merged

wfurt mentioned this pull request Jul 14, 2020

Socket.ConnectAsync result get the socket error "InvalidArgument" for UDP #39268

Closed

ghost locked as resolved and limited conversation to collaborators Dec 10, 2020

Conversation

tmds commented Mar 17, 2020

Uh oh!

antonfirsov left a comment

Choose a reason for hiding this comment

Uh oh!

davidsh commented Mar 17, 2020

Uh oh!

Uh oh!

scalablecory left a comment

Choose a reason for hiding this comment

Uh oh!

antonfirsov commented Mar 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

antonfirsov commented Mar 21, 2020

Uh oh!

azure-pipelines bot commented Mar 21, 2020

Uh oh!

antonfirsov commented Mar 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tmds Mar 21, 2020

Choose a reason for hiding this comment

Uh oh!

scalablecory Mar 21, 2020

Choose a reason for hiding this comment

Uh oh!

tmds Mar 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmds commented Mar 21, 2020

Uh oh!

tmds commented Mar 31, 2020

Uh oh!

antonfirsov commented Mar 31, 2020

Uh oh!

tmds commented Apr 2, 2020

Uh oh!

scalablecory commented Apr 2, 2020

Uh oh!

antonfirsov commented Apr 2, 2020

Uh oh!

azure-pipelines bot commented Apr 2, 2020

Uh oh!

antonfirsov Apr 3, 2020

Choose a reason for hiding this comment

Uh oh!

tmds Apr 3, 2020

Choose a reason for hiding this comment

Uh oh!

antonfirsov left a comment

Choose a reason for hiding this comment

Uh oh!

scalablecory left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

antonfirsov commented Mar 19, 2020 •

edited

Loading

antonfirsov commented Mar 21, 2020 •

edited

Loading

tmds Mar 21, 2020 •

edited

Loading