Enable instance Socket.{Begin}Connect{Async} methods on Unix by stephentoub · Pull Request #16373 · dotnet/corefx

stephentoub · 2017-02-22T04:18:57Z

BSD Sockets don't allow using a socket for a connection that has already failed in a previous connect. This has caused us to significantly hamper the Socket.{Begin}Connect{Async} instance methods on Socket, in two ways:

If you call any instance connect overload and it fails, whereas on Windows you can try to connect again, on Unix we throw a PlatformNotSupportedException.
If you call any instance connect method that could potentially try to connect to multiple addresses (e.g. a string host, an IPAddress[], or a DnsEndPoint), whereas on Windows that allowed, on Unix we throw a PlatformNotSupportedException (because after trying the first address we will then be trying to connect again on a socket that failed to connect).

There are multiple options for dealing with this, all with their own pros and cons:
A) Use a "shadow" socket to first try to make the connection, and if it succeeds, assume that another connection will succeed and try to make it on the "real" socket. We actually originally went down this path prior to .NET Core 1.0 and it had significant problems, both with performance (multiple connections any time a single one was needed) and with server expectations (getting two connections when only one was expected).
B) Prohibit usage of any {Begin}Connect{Async} APIs that could potentially deal with multiple addresses. This is what we've shipped in .NET Core 1.x.
C) In such APIs, check whether the number of addresses is exactly 1, allowing it if there is, throwing otherwise. This has several issues, including problems with dual mode (e.g. localhost will often resolve to both ::1 and 127.0.0.1), non-determinism based on how many addresses a server resolves to, etc.
D) Regardless of the number of addresses, pretend there was only one. This has similar issues to (C).
E) Use a "shadow" socket to make each connection, and then on successful connection substitute it in for the original underneath the Socket. This enables trying to connect to multiple addresses, but it means that any configuration performed on the initial socket won't be applied to the subsequent sockets.
F) Same as (E), but keeping track of some dialable set of options that can be propagated from socket to socket.

This commit implements (F). (F) still isn't perfect, but it allows the common path (minimal configuration) to work while still supporting multiple endpoints and being deterministic:

SafeCloseSocket on Unix now tracks whether the Socket.Handle property was accessed and whether any non-tracked configuration was set. If any of that is done, then as with today. We were already tracking (in order to provide an error message) whether a connect attempt failed; that was previously tracked on the SocketAsyncContext, now it's tracked on the SafeCloseSocket handle.
Whenever an attempt is made to {Begin}Connect{Async} with multiple possible endpoints, we check whether the handle was exposed or untracked configuration was used. If it was, we fail as we do today. If it wasn't, we allow it.
When making a connection (either via a user's call, or as part of a multi-endpoint connect operation), if there was a previously failed connection attempt, we replace the handle with a new one, transferring over the tracked state from the old handle to the new one.
Removed a bunch of stale code leftover from when approach (A) above was implemented; apparently when we switched to (B) this code was never removed
Removed now defunct [PlatformSpecific] attribution, and added some more tests

With (F), we also no longer need the workaround we had in TcpClient, so I recombined it back down to a single implementation built on top of Socket. This means, for example, that whereas previously on Unix you couldn't use TcpClient's instance Connect methods with a DNS endpoint after accessing its Client Socket, now you can. And we don't have to do any of the shadow value tracking we were previously doing.

In addition, I took care of some other minor cleanup I spotted along the way:

One or two cases where API usage was ifdef'd out; now that these APIs are back for 2.0, we can use them again, e.g. IPAddress.Address
Fixed a few places where exception stacks were being lost due to exceptions getting rethrown; changed these to use ExceptionDispatchInfo to maintain the stacks (this helped me to debug a few things better, which is how I noticed in the first place)
Removed unnecessary pragma usage that was a holdover from older versions of the code
Adding code to a bunch of tests to Dispose of sockets
Adding a few more tests (there's still a lot of uncovered code in Sockets unfortunately; we really need to address that, separately)

Fixes https://github.com/dotnet/corefx/issues/8768
cc: @NickCraver, @tmds, @glennc, @geoffkizer, @danroth27, @richlander, @pgavlin, @halter73

stephentoub · 2017-02-22T04:21:10Z

@dotnet-bot test outerloop CentOS7.1 Debug
@dotnet-bot test outerloop OpenSUSE42.1 Debug
@dotnet-bot test outerloop PortableLinux Debug
@dotnet-bot test outerloop RHEL7.2 Debug
@dotnet-bot test outerloop Ubuntu14.04 Release
@dotnet-bot test outerloop Ubuntu16.10 Debug
@dotnet-bot test outerloop OSX Debug
@dotnet-bot test outerloop Windows 10 Debug

stephentoub · 2017-02-23T12:06:37Z

@geoffkizer, @pgavlin, @CIPop, could you guys help review this?

NickCraver

This is awesome - the huge amount of effort here is much appreciated. I left 2 minor comments on a nit and connect allocations for multi-family, otherwise very much looking forward to this landing.

NickCraver · 2017-02-23T01:32:30Z

+            // replicating the socket's file descriptor appropriately.  Similarly, if it's
+            // only targeting a single address, but it already experienced a failure in a
+            // previous connect call, then this is logically part of a multi endpoint connect,
+            // and the same logic applies.  Either wya, in such a situation we throw.


NickCraver · 2017-02-23T12:56:40Z

+                    if (Socket.OSSupportsIPv6)
+                    {
+                        ipv6Socket = new Socket(AddressFamily.InterNetworkV6, SocketType.Stream, ProtocolType.Tcp);
+                    }


Should this spin up both early, or do so more lazily underneath? e.g. if the first address is one family, we're allocating a socket for the other family needlessly. I'd imagine that's the majority case - the first attempt succeeding.

Probably. This is how it was in Windows and in desktop, but I don't currently see any reason why it can't be made lazy. Will fix.

geoffkizer · 2017-02-24T02:09:18Z

+            if (_handle.IsTrackedOption(TrackedSocketOptions.ReceiveTimeout)) receiveTimeout = ReceiveTimeout;
+            if (_handle.IsTrackedOption(TrackedSocketOptions.SendBufferSize)) sendSize = SendBufferSize;
+            if (_handle.IsTrackedOption(TrackedSocketOptions.SendTimeout)) sendTimeout = SendTimeout;
+            if (_handle.IsTrackedOption(TrackedSocketOptions.Ttl)) ttl = Ttl;


Why read all the values out, create the new socket, and then put all the values in the new one? Seems like it would be simpler to create the new socket and then copy each one individually.

All of these options are properties on Socket, some with a fair amount of non-trivial code behind them. Getting the value is easy via the property, but doing so accesses the underlying handle, and there can of course be only one handle at a time. So to do what you propose, I'd need to skip using these properties and instead duplicate all of the code necessary to get/set all of these directly against the handle.

Ah, now I understand. Thanks.

geoffkizer · 2017-02-24T02:17:10Z

                        break;
                    }
-                    catch (Exception ex)
+                    catch (Exception ex) when (!ExceptionCheck.IsFatal(ex))


Do we need the check for IsFatal? I thought we were going to get rid of this.

I'd be fine deleting them all in a separate pass (IIRC at this point IsFatal really just checks for OOMs). For now I'll keep it to avoid changing any of that logic in this PR; all I did here was take advantage of exception filters to neaten up the code slightly.

Ok by me. Separately, do you think these should be deleted? If so can we file an issue?

geoffkizer · 2017-02-24T02:35:35Z

            if (error != SocketError.Success && error != SocketError.WouldBlock)
            {
-                _connectFailed = true;
+                _socket.LastConnectFailed = true;


Since this function modifies state on the SafeCloseSocket, would it make more sense for it to live on that class?

Probably. I'll move it unless there's any good reason to keep it here.

geoffkizer · 2017-02-24T02:47:10Z

Generally LGTM. A few nits above.

stephentoub · 2017-02-24T03:06:22Z

Thanks for the reviews, @NickCraver and @geoffkizer.

BSD Sockets don't allow using a socket for a connection that has already failed in a previous connect. This has caused us to significantly hamper the Socket.{Begin}Connect{Async} instance methods on Socket, in two ways: 1) If you call any instance connect overload and it fails, whereas on Windows you can try to connect again, on Unix we throw a PlatformNotSupportedException. 2) If you call any instance connect method that could potentially try to connect to multiple addresses (e.g. a string host, an IPAddress[], or a DnsEndPoint), whereas on Windows that allowed, on Unix we throw a PlatformNotSupportedException (because after trying the first address we will then be trying to connect again on a socket that failed to connect). There are multiple options for dealing with this, all with their own pros and cons: A) Use a "shadow" socket to first try to make the connection, and if it succeeds, assume that another connection will succeed and try to make it on the "real" socket. We actually originally went down this path prior to .NET Core 1.0 and it had significant problems, both with performance (multiple connections any time a single one was needed) and with server expectations (getting two connections when only one was expected). B) Prohibit usage of any {Begin}Connect{Async} APIs that could potentially deal with multiple addresses. This is what we've shipped in .NET Core 1.x. C) In such APIs, check whether the number of addresses is exactly 1, allowing it if there is, throwing otherwise. This has several issues, including problems with dual mode (e.g. localhost will often resolve to both ::1 and 127.0.0.1), non-determinism based on how many addresses a server resolves to, etc. D) Regardless of the number of addresses, pretend there was only one. This has similar issues to (C). E) Use a "shadow" socket to make each connection, and then on successful connection substitute it in for the original underneath the Socket. This enables trying to connect to multiple addresses, but it means that any configuration performed on the initial socket won't be applied to the subsequent sockets. F) Same as (E), but keeping track of some dialable set of options that can be propagated from socket to socket. This commit implements (F). (F) still isn't perfect, but it allows the common path (minimal configuration) to work while still supporting multiple endpoints and being deterministic: - SafeCloseSocket on Unix now tracks whether the Socket.Handle property was accessed and whether any non-tracked configuration was set. If any of that is done, then as with today. We were already tracking (in order to provide an error message) whether a connect attempt failed; that was previously tracked on the SocketAsyncContext, now it's tracked on the SafeCloseSocket handle. - Whenever an attempt is made to {Begin}Connect{Async} with multiple possible endpoints, we check whether the handle was exposed or untracked configuration was used. If it was, we fail as we do today. If it wasn't, we allow it. - When making a connection (either via a user's call, or as part of a multi-endpoint connect operation), if there was a previously failed connection attempt, we replace the handle with a new one, transferring over the tracked state from the old handle to the new one. - Removed a bunch of stale code leftover from when approach (A) above was implemented; apparently when we switched to (B) this code was never removed - Removed now defunct [PlatformSpecific] attribution, and added some more tests In addition, I took care of some other minor cleanup I spotted along the way: - One or two cases where API usage was ifdef'd out; now that these APIs are back for 2.0, we can use them again, e.g. IPAddress.Address - Fixed a few places where exception stacks were being lost due to exceptions getting rethrown; changed these to use ExceptionDispatchInfo to maintain the stacks (this helped me to debug a few things better, which is how I noticed in the first place) - Removed unnecessary pragma usage that was a holdover from older versions of the code - Adding code to a bunch of tests to Dispose of sockets

We previously had to specialize TcpClient for Unix to enable {Begin}Connect{Async} to support multiple endpoints. Now that Socket does implicitly, we can remove that specialization, and have a single implementation for TcpClient (TcpClient only exposes surface area to tweak configuration tracked by the Unix Socket implementation). In doing so, this further opens up TcpClient usage on Unix, e.g. previously accessing the Client Socket prior to calling Connect would disable the ability to use multiple endpoints, but now it works.

stephentoub · 2017-02-25T17:31:01Z

@dotnet-bot test outerloop CentOS7.1 Debug
@dotnet-bot test outerloop OpenSUSE42.1 Debug
@dotnet-bot test outerloop PortableLinux Debug
@dotnet-bot test outerloop RHEL7.2 Debug
@dotnet-bot test outerloop Ubuntu14.04 Release
@dotnet-bot test outerloop Ubuntu16.10 Debug
@dotnet-bot test outerloop OSX Debug
@dotnet-bot test outerloop Windows 10 Debug

stephentoub · 2017-02-25T19:19:12Z

@dotnet-bot test outerloop PortableLinux Debug (https://github.com/dotnet/corefx/issues/16201)

Enable instance Socket.{Begin}Connect{Async} methods on Unix Commit migrated from dotnet/corefx@969d868

dnfclas added the cla-already-signed label Feb 22, 2017

karelz added the area-System.Net.Sockets label Feb 22, 2017

karelz assigned stephentoub Feb 22, 2017

NickCraver reviewed Feb 23, 2017

View reviewed changes

geoffkizer reviewed Feb 24, 2017

View reviewed changes

stephentoub added 3 commits February 25, 2017 11:17

Address PR feedback

bb0e166

stephentoub force-pushed the socket_connect branch from 6dd2067 to bb0e166 Compare February 25, 2017 16:17

stephentoub merged commit 969d868 into dotnet:master Feb 25, 2017

stephentoub deleted the socket_connect branch February 25, 2017 21:57

karelz modified the milestone: 2.0.0 Mar 1, 2017

stephentoub mentioned this pull request Mar 11, 2017

Add support for System.Net.Sockets.Socket.Disconnect on Unix #17000

Merged

NickCraver mentioned this pull request Mar 22, 2017

Workaround for connecting redis via host name on Linux StackExchange/StackExchange.Redis#544

Closed

mgravell mentioned this pull request Jun 2, 2017

AWS Lambda Function not able to connect to redis instance via StackExchange.Redis StackExchange/StackExchange.Redis#640

Closed

cocowalla mentioned this pull request Jan 31, 2020

PlatformNotSupportedException with TcpClient.ConnectAsync dotnet/runtime#24917

Closed

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/corefx#16373 from stephentoub/socket_connect

8f5b479

Enable instance Socket.{Begin}Connect{Async} methods on Unix Commit migrated from dotnet/corefx@969d868

Uh oh!

Conversation

stephentoub commented Feb 22, 2017

Uh oh!

stephentoub commented Feb 22, 2017

Uh oh!

stephentoub commented Feb 23, 2017

Uh oh!

NickCraver left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geoffkizer commented Feb 24, 2017

Uh oh!

stephentoub commented Feb 24, 2017

Uh oh!

stephentoub commented Feb 25, 2017

Uh oh!

stephentoub commented Feb 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants