Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Commit

Permalink
Disable Unix workarounds for Connect'ing to multiple endpoints
Browse files Browse the repository at this point in the history 
On unix, once connect fails on a socket, that socket becomes unusable for further operations, including additional connect attempts.  This is at direct odds with Socket's instance Connect methods, some of which allow for multiple connect attempts, either due to multiple IPAddresses being provided or due to a string hostname / DnsEndPoint being provided that could then result in multiple IPAddresses to be tried.

We've explored multiple workarounds for this, all of which have problems.  The workaround still in the code involves creating a temporary socket for each address, connecting to it, and if that's successful then immediately disconnecting and connecting with the actual socket.  But that causes mayhem for a server not expecting that pattern, e.g. that fails if the client disconnects and attempts a reconnect.

This leaves us with a few choices, none of which are great:
1. Remove the offending Connect instance methods.  Ideally they'd be replaced with static methods, which can be implemented with POSIX-compliant behavior.  But these methods are heavily used and work on Windows.
2. Always throw from the instance Connect methods when there's a possibility that multiple addresses will be tried, e.g. from Connect(IPAddress[], ...) but also from Connect(EndPoint) if a DnsEndPoint is specified.  This will break a lot of existing code, but it's also predictable, and developers will know quickly when using a problematic API and move away from it to supported patterns.
3. Throw from the Connect methods if multiple addresses are actually supplied, e.g. calling Connect(IPAddress[]) with an array of length 1 would work but an array of length 2 will fail. This will allow a lot more existing code to work, but it's also very unpredictable, e.g. if a string host is provided and gets mapped by DNS to a single IPAddress in the test environment but then multiple IPAddresses in the production environment, everything will work fine locally but then fail in production.
4. When presented with multiple addresses, try the first one, and if it fails, then throw a PlatformNotSupportedException.  This may be slightly more predictable than (3), but is still unpredictable, e.g. if a DNS server returns addresses in a different quantity or order from another server.

I'm torn between (2) and (3) (and maybe (4)).  This commit implements the more conservative (2), as I expect (3) would actually cause serious problems for deployments, but we can loosen from (2) to (3) if that turns out to be better.  In the meantime, we should also seriously explore adding static methods for v1 for the same functionality (developers can also write such code themselves), and write guidance and helpers folks can use in the meantime, e.g. pseudo-code:
```C#
public static async Task<Socket> Connect(
    AddressFamily addressFamily, SocketType socketType, ProtocolType protocolType,
    IPAddress[] addresses, int port)
{
    Exception lastExc = null;
    foreach (IPAddress address in addresses)
    {
        Socket s = new Socket(addressFamily, socketType, protocolType);
        try
        {
            await s.ConnectAsync(address, port).ConfigureAwait(false);
            return s;
        }
        catch (Exception exc)
        {
            lastExc = exc;
        }
    }

    if (lastExc != null) throw lastExc;
    throw new ArgumentException("No addresses provided", "addresses");
}
```
  • Loading branch information
@stephentoub
stephentoub committed Feb 4, 2016
1 parent 11cb6e8 commit 30bd4b7
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 65 deletions.
3 changes: 3 additions & 0 deletions src/System.Net.Sockets/src/Resources/Strings.resx
View file
Expand Up @@ -273,4 +273,7 @@
<data name="ArgumentOutOfRange_Bounds_Lower_Upper" xml:space="preserve">
<value>Argument must be between {0} and {1}.</value>
</data>
<data name="net_sockets_connect_multiaddress_notsupported" xml:space="preserve">
<value>Sockets on this platform are invalid for use after a failed connection attempt, and as a result do not support attempts to connect to multiple endpoints. Use the static Connect methods or the instance Connect methods that take a single, non-DNS endpoint.</value>
</data>
</root>
87 changes: 22 additions & 65 deletions src/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
View file
Expand Up @@ -980,77 +980,18 @@ public void Connect(IPAddress[] addresses, int port)
{
throw new NotSupportedException(SR.net_invalidversion);
}
ThrowIfNotSupportsMultipleConnectAttempts();

// Disable CS0162: Unreachable code detected
//
// SuportsMultipleConnectAttempts is a constant; when false, the following lines will trigger CS0162.
#pragma warning disable 162
Exception lastex = null;
if (SocketPal.SupportsMultipleConnectAttempts)
foreach (IPAddress address in addresses)
{
foreach (IPAddress address in addresses)
{
if (CanTryAddressFamily(address.AddressFamily))
{
try
{
Connect(new IPEndPoint(address, port));
lastex = null;
break;
}
catch (Exception ex)
{
if (ExceptionCheck.IsFatal(ex))
{
throw;
}
lastex = ex;
}
}
}
}
else
{
EndPoint endpoint = null;
foreach (IPAddress address in addresses)
{
if (CanTryAddressFamily(address.AddressFamily))
{
Socket attemptSocket = null;
try
{
attemptSocket = new Socket(_addressFamily, _socketType, _protocolType);
if (IsDualMode)
{
attemptSocket.DualMode = true;
}

var attemptEndpoint = new IPEndPoint(address, port);
attemptSocket.Connect(attemptEndpoint);
endpoint = attemptEndpoint;
lastex = null;
break;
}
catch (Exception ex)
{
lastex = ex;
}
finally
{
if (attemptSocket != null)
{
attemptSocket.Dispose();
}
}
}
}

if (endpoint != null)
if (CanTryAddressFamily(address.AddressFamily))
{
try
{
Connect(endpoint);
Connect(new IPEndPoint(address, port));
lastex = null;
break;
}
catch (Exception ex)
{
Expand All @@ -1062,7 +1003,6 @@ public void Connect(IPAddress[] addresses, int port)
}
}
}
#pragma warning restore

if (lastex != null)
{
Expand Down Expand Up @@ -2415,6 +2355,8 @@ internal IAsyncResult BeginConnect(string host, int port, AsyncCallback requestC
throw new InvalidOperationException(SR.net_sockets_mustnotlisten);
}

ThrowIfNotSupportsMultipleConnectAttempts();

// Here, want to flow the context. No need to lock.
MultipleAddressConnectAsyncResult result = new MultipleAddressConnectAsyncResult(null, port, this, state, requestCallback);
result.StartPostingAsyncOp(false);
Expand All @@ -2438,6 +2380,14 @@ internal IAsyncResult BeginConnect(string host, int port, AsyncCallback requestC
return result;
}

private static void ThrowIfNotSupportsMultipleConnectAttempts()
{
if (!SocketPal.SupportsMultipleConnectAttempts)
{
throw new PlatformNotSupportedException(SR.net_sockets_connect_multiaddress_notsupported);
}
}

internal IAsyncResult BeginConnect(IPAddress address, int port, AsyncCallback requestCallback, object state)
{
if (s_loggingEnabled)
Expand Down Expand Up @@ -2502,6 +2452,7 @@ internal IAsyncResult BeginConnect(IPAddress[] addresses, int port, AsyncCallbac
{
throw new InvalidOperationException(SR.net_sockets_mustnotlisten);
}
ThrowIfNotSupportsMultipleConnectAttempts();

// Set up the result to capture the context. No need for a lock.
MultipleAddressConnectAsyncResult result = new MultipleAddressConnectAsyncResult(addresses, port, this, state, requestCallback);
Expand Down Expand Up @@ -4408,6 +4359,8 @@ public bool ConnectAsync(SocketAsyncEventArgs e)
throw new NotSupportedException(SR.net_invalidversion);
}

ThrowIfNotSupportsMultipleConnectAttempts();

MultipleConnectAsync multipleConnectAsync = new SingleSocketMultipleConnectAsync(this, true);

e.StartOperationCommon(this);
Expand Down Expand Up @@ -4518,6 +4471,10 @@ public static bool ConnectAsync(SocketType socketType, ProtocolType protocolType
// Disable CS0162 and CS0429: Unreachable code detected
//
// SuportsMultipleConnectAttempts is a constant; when false, the following lines will trigger CS0162 or CS0429.
//
// This is the only *Connect* API that actually supports multiple endpoint attempts, as it's responsible
// for creating each Socket instance and can create one per attempt (with the instance methods, once a
// connect fails, on unix systems that socket can't be used for subsequent connect attempts).
#pragma warning disable 162, 429
multipleConnectAsync = SocketPal.SupportsMultipleConnectAttempts ?
(MultipleConnectAsync)(new DualSocketMultipleConnectAsync(socketType, protocolType)) :
Expand Down

0 comments on commit 30bd4b7

Please sign in to comment.