add support for parallel Connect #106374

wfurt · 2024-08-14T00:15:18Z

this adds API approved in #87932. It seems like this only supports SAEA. We may consider extending it to be also supported with Task.

To limit possible risk, I added new member to existing and transient MultiConnectSocketAsyncEventArgs e.g. there should not be any penalty for Socket. We also originally discussed some split DNS query with @liveans and that can be possibly done separately. This is basic snapshot I crafted when hoping to squeeze this to 9.0.

Testing is tricky as it depends on DNS entries and particular behavior and speed.

dotnet-issue-labeler · 2024-08-14T00:15:24Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

dotnet-issue-labeler · 2024-08-14T00:15:26Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

dotnet-policy-service · 2024-08-14T00:15:49Z

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

liveans · 2024-08-14T00:28:42Z

I'm not sure about the behavior of Dns.Get* APIs with AddressFamily.Unspecified.
If it blocks until we get a response from both AddressFamily, then this might be problem here.

Because if either IPv6/IPv4 response is late or timeouts somehow, then it means we're delaying connection until we got both of the responses, this can be problematic for some specific cases.

But on the other hand with this approach, OS is implementing Destination Address Selection Algorithm for us, and we don't have code complexity here, if we decide to go to splitting DNS queries we might need to consider implementing Destination Address Selection Algorithm.

gfoidl · 2024-08-14T19:30:15Z

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

+                        }
+                        else
+                        {
+                            if (nextAddressIndex == -1 && nextIPv6Addressindex == -1)


Suggested change

if (nextAddressIndex == -1 && nextIPv6Addressindex == -1)

if (nextAddressIndex < 0 && nextIPv6Addressindex < 0)

The JIT can produce less and more optimized machine-code (if it matters here too, but the change is cheap...)

gfoidl · 2024-08-14T19:32:42Z

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

@@ -833,13 +1001,13 @@ async Task Core(MultiConnectSocketAsyncEventArgs internalArgs, Task<IPAddress[]>
                    // Store the results.
                    if (caughtException != null)
                    {
-                        SetResults(caughtException, 0, SocketFlags.None);
+                         SetResults(caughtException, 0, SocketFlags.None);


Nit: there's a space too much

Suggested change

SetResults(caughtException, 0, SocketFlags.None);

SetResults(caughtException, 0, SocketFlags.None);

liveans · 2024-08-17T00:27:19Z

src/libraries/System.Net.Sockets/tests/FunctionalTests/SocketAsyncEventArgsTest.cs

@@ -1087,6 +1087,84 @@ public async Task SendTo_DifferentEP_Success(bool ipv4)
            result = await receiver2.ReceiveFromAsync(receiveBuffer, remoteEp).WaitAsync(TestSettings.PassingTestTimeout);
            Assert.Equal(sendBuffer.Length, result.ReceivedBytes);
        }
+
+        [ConditionalFact(typeof(DualModeBase), nameof(DualModeBase.LocalhostIsBothIPv4AndIPv6))]
+        public void Connect_Parallel_Succss()


Suggested change

public void Connect_Parallel_Succss()

public void Connect_Parallel_Success()

liveans · 2024-08-23T09:32:08Z

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

+
+                    int nextAddressIndex = 0;
+                    int nextIPv6AddressIndex = GetNextAddress(addresses, AddressFamily.InterNetworkV6, -1);
+                    int nextUPv4AddressIndex = GetNextAddress(addresses, AddressFamily.InterNetwork, -1);


Looks like this variable only used within parallelConnect variable and that's it, perhaps delete this and call function directly in parallelConnect.

liveans · 2024-08-23T09:34:04Z

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

+                                {
+                                    address = addresses[nextAddressIndex];
+                                    nextAddressIndex = GetNextAddress(addresses, currentAddressFamily, nextAddressIndex);
+


Suggested change

liveans · 2024-08-23T09:34:29Z

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

+                                }
+                                    internalArgs.SocketError = SocketError.IOPending;


Suggested change

}

internalArgs.SocketError = SocketError.IOPending;

}

internalArgs.SocketError = SocketError.IOPending;

liveans · 2024-08-23T09:36:25Z

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

+                                        address = addresses[nextIPv6AddressIndex];
+                                        internalArgs.SocketError = SocketError.IOPending;
+                                        address2 = null;
+                                        internalArgs.SecondarySaea = null;


Should we dispose this before null out?

liveans · 2024-08-23T09:37:47Z

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

+                            {
+                                if (nextIPv6AddressIndex >= 0)
+                                {
+                                    address2 = addresses[nextIPv6AddressIndex];


There is duplicate assign on line 806

liveans · 2024-08-23T09:40:57Z

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

+                                break;
+                            }
+
+                            if (internalArgs.SocketError != SocketError.IOPending)


Can we be more specific with this check, and propagate the error if the error is something else other than expected (e.g. SocketError.SystemNotReady) ?

liveans · 2024-08-23T09:44:55Z

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

+                                if (nextIPv6AddressIndex >= 0)
+                                {
+                                    address2 = addresses[nextIPv6AddressIndex];
+                                    if (address == null)


Suggested change

if (address == null)

if (address is not null)

stephentoub · 2024-09-05T17:53:30Z

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

+        // defaul mechanism e.g. sequential processing
+        Default = 0,
+
+        // use a Happy Eyeballs-like algorithm to connect.


Nit: Please use XML comments

stephentoub · 2024-09-05T17:54:03Z

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

-        public static bool ConnectAsync(SocketType socketType, ProtocolType protocolType, SocketAsyncEventArgs e)
+        public static bool ConnectAsync(SocketType socketType, ProtocolType protocolType, SocketAsyncEventArgs e) =>
+                            ConnectAsync(socketType, protocolType,  e, ConnectAlgorithm.Default);
+        public static bool ConnectAsync(SocketType socketType, ProtocolType protocolType, SocketAsyncEventArgs e, ConnectAlgorithm connectAlgorithm)


Validate that connectAlgorithm is one of the two known values?

stephentoub · 2024-09-05T17:55:54Z

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

+                    int nextUPv4AddressIndex = GetNextAddress(addresses, AddressFamily.InterNetwork, -1);
+
+
+                    // We can do parallel connect only if siocket was not specified and when there is at least one address of each AF.


Suggested change

// We can do parallel connect only if siocket was not specified and when there is at least one address of each AF.

// We can do parallel connect only if socket was not specified and when there is at least one address of each AF.

stephentoub · 2024-09-05T17:56:23Z

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

                    {
+                        if (!parallelConnect)
+                        {
+                            // We simply try addresses in sequence until we either try them all or operation is cancelled.


Nit:

Suggested change

// We simply try addresses in sequence until we either try them all or operation is cancelled.

// We simply try addresses in sequence until we either try them all or operation is canceled.

stephentoub · 2024-09-05T17:57:33Z

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

@@ -672,12 +672,27 @@ internal void FinishOperationAsyncFailure(SocketError socketError, int bytesTran
            }
        }

+        private static int GetNextAddress(IPAddress[] addresses, AddressFamily addressFamily, int index)
+        {
+            for (int i = index + 1; i < addresses.Length; i++)


Why the +1 and then using -1 later?

add support for parallel Connect

ffd7ac4

wfurt added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) area-System.Net.Sockets labels Aug 14, 2024

wfurt added this to the 10.0.0 milestone Aug 14, 2024

wfurt requested a review from a team August 14, 2024 00:15

dotnet-issue-labeler bot added the new-api-needs-documentation label Aug 14, 2024

dotnet-policy-service bot assigned wfurt Aug 14, 2024

gfoidl reviewed Aug 14, 2024

View reviewed changes

wfurt added 3 commits August 20, 2024 11:02

bind

6ed4712

Merge branch 'main' of https://github.com/dotnet/runtime into happyEye

759a0da

feedback

ce5589b

liveans reviewed Aug 26, 2024

View reviewed changes

stephentoub reviewed Sep 5, 2024

View reviewed changes

wfurt removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Sep 9, 2024

wfurt marked this pull request as draft September 10, 2024 01:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for parallel Connect #106374

add support for parallel Connect #106374

wfurt commented Aug 14, 2024

dotnet-issue-labeler bot commented Aug 14, 2024

dotnet-issue-labeler bot commented Aug 14, 2024

dotnet-policy-service bot commented Aug 14, 2024

liveans commented Aug 14, 2024

gfoidl Aug 14, 2024

gfoidl Aug 14, 2024

liveans Aug 17, 2024

liveans Aug 23, 2024

liveans Aug 23, 2024

liveans Aug 23, 2024

liveans Aug 23, 2024

liveans Aug 23, 2024

liveans Aug 23, 2024

liveans Aug 23, 2024

stephentoub Sep 5, 2024

stephentoub Sep 5, 2024

stephentoub Sep 5, 2024

stephentoub Sep 5, 2024

stephentoub Sep 5, 2024

	if (nextAddressIndex == -1 && nextIPv6Addressindex == -1)
	if (nextAddressIndex < 0 && nextIPv6Addressindex < 0)

	SetResults(caughtException, 0, SocketFlags.None);
	SetResults(caughtException, 0, SocketFlags.None);

	public void Connect_Parallel_Succss()
	public void Connect_Parallel_Success()

		int nextUPv4AddressIndex = GetNextAddress(addresses, AddressFamily.InterNetwork, -1);


		// We can do parallel connect only if siocket was not specified and when there is at least one address of each AF.

	// We can do parallel connect only if siocket was not specified and when there is at least one address of each AF.
	// We can do parallel connect only if socket was not specified and when there is at least one address of each AF.

	// We simply try addresses in sequence until we either try them all or operation is cancelled.
	// We simply try addresses in sequence until we either try them all or operation is canceled.

add support for parallel Connect #106374

Are you sure you want to change the base?

add support for parallel Connect #106374

Conversation

wfurt commented Aug 14, 2024

dotnet-issue-labeler bot commented Aug 14, 2024

dotnet-issue-labeler bot commented Aug 14, 2024

dotnet-policy-service bot commented Aug 14, 2024

liveans commented Aug 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment