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

Rework Unix socket option tracking #111676

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

antonfirsov
Copy link
Member

@antonfirsov antonfirsov commented Jan 21, 2025

This PR changes how socket option tracking and state transfer is implemented to support multi-connect on Unix. Instead of calling property setters and getters, the new logic uses GetSocketOption / SetSocketOption directly and extends the set of tracked options. The property logic did two things: validations and and the selection of the the necessary SocketOptionLevel. Regarding the former: when transferring the state, validation is unnecessary, since it already happened in the property setter of the original socket. Regarding the latter: SocketOptionLevel is now implied by TrackableSocketOptions. To make sure there is no mistake in the mapping, I generated the necessary mapping logic programatically based on option names/values taken from Windows headers: https://gist.github.com/antonfirsov/2cbfc37e665ad840ed7734994948c29a

The new implementation now tracks more options, however options which are specific to datagram/udp sockets were removed from the set of tracked options, since such sockets will never be multi-connected in practice. If we think we need to add some of them back, it is easy to do so since it is easy to extend the new tracking logic to other known options of int values.

Additionally, the PR relaxes the check done in ValidateForMultiConnect: if the socket handle's state is still transferrable (ExposedHandleOrUntrackedConfiguration == false), we no longer throw from multi-endpoint API-s at the beginning of the call. If the first connect attempt fails and the handle is exposed, we terminate with the exception of the first failure, ignoring the remaining endpoints.

Fixes #110124

@liveans
Copy link
Member

liveans commented Feb 4, 2025

I started to look at this today, planning to finish reviewing soon.

Copy link
Member

@MihaZupan MihaZupan left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me, thank you

@@ -209,7 +209,7 @@ async ValueTask Core(IPAddress[] addresses, int port, CancellationToken cancella
await ConnectAsync(endPoint, cancellationToken).ConfigureAwait(false);
return;
}
catch (Exception ex) when (ex is not OperationCanceledException)
catch (Exception ex) when (CanProceedWithMultiConnect && ex is not OperationCanceledException)
Copy link
Member

Choose a reason for hiding this comment

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

The cases where this changes behavior are ones where things may have looked as if they were working, but we weren't copying all the settings even if we did manage to connect, right?
That is, for the edge case of options that we aren't tracking even with this change.

Copy link
Member Author

@antonfirsov antonfirsov Feb 5, 2025

Choose a reason for hiding this comment

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

Yes, with this change we would correctly throw in such cases, but my assumption is that those should be quite rare. However, we would also start throwing from this method when the user manually touches Socket.SafeHandle (and the first connect attempt in the multiconnect series fails), maybe that should qualify this PR as a breaking?

Copy link
Member

Choose a reason for hiding this comment

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

we would also start throwing from this method when the user manually touches Socket.SafeHandle

That's a good point. I suppose it's all part of the existing inconsistencies.

If I run something like this Test.cs, I currently see this on Unix:

[PASS]: async s => s.Connect("mihubot.xyz", 443)
[FAIL]: exposed async s => s.Connect("mihubot.xyz", 443)

[PASS]: async s => s.Connect(Dns.GetHostAddresses("mihubot.xyz"), 443)
[FAIL]: exposed async s => s.Connect(Dns.GetHostAddresses("mihubot.xyz"), 443)

[PASS]: async s => s.Connect(Dns.GetHostAddresses("mihubot.xyz")[0], 443)
[PASS]: exposed async s => s.Connect(Dns.GetHostAddresses("mihubot.xyz")[0], 443)

[PASS]: async s => await s.ConnectAsync("mihubot.xyz", 443)
[FAIL]: exposed async s => await s.ConnectAsync("mihubot.xyz", 443)

[PASS]: async s => await s.ConnectAsync(Dns.GetHostAddresses("mihubot.xyz"), 443)
[PASS]: exposed async s => await s.ConnectAsync(Dns.GetHostAddresses("mihubot.xyz"), 443)

[PASS]: async s => await s.ConnectAsync(Dns.GetHostAddresses("mihubot.xyz")[0], 443)
[PASS]: exposed async s => await s.ConnectAsync(Dns.GetHostAddresses("mihubot.xyz")[0], 443)

[PASS]: SocketAsyncEventArgs
[FAIL]: exposed SocketAsyncEventArgs

Do I understand it correctly that with this change, all of these would succeed (if the handle is exposed we'll still try at least the first address)?

Copy link
Member

@liveans liveans left a comment

Choose a reason for hiding this comment

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

LGTM modulo existing comments

@antonfirsov
Copy link
Member Author

/azp run runtime-libraries-coreclr outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Socket on Unix behaves inconsistently between IPAddress[] and DnsEndPoint connects
3 participants