Skip to content

Restore Socket.ConnectAsync via hostname on *nix platforms. #17374

Description

@NickCraver

There's a bit of history to this, but due to https://github.com/dotnet/corefx/issues/5829 which was resolved by dotnet/corefx@30bd4b7, we are no longer able to use hostnames in socket connections for some platforms. Currently, it's a runtime PlatformNotSupportedException.

This breaks StackExchange.Redis on at least OS X and Linux, you can see the issue filed here: StackExchange/StackExchange.Redis#410

The best notes for this are in the dotnet/corefx@30bd4b7 commit, copied here for ease of consumption:

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.

Currently option 2 is implemented, which breaks this codepath:
SocketTaskExtensions.ConnectAsync(this Socket socket, string host, int port)
Socket.BeginConnect(string host, int port, AsyncCallback requestCallback, object state)

This hits line 2334: ThrowIfNotSupportsMultipleConnectAttempts();

The exact codepath doesn't matter, because several more feed into this one. In fact even if you call BeginConnect(EndPoint remoteEP, AsyncCallback callback, object state), it's still calling BeginConnect(dnsEP.Host, dnsEP.Port, callback, state); in the host case anyway. So even using the explicit overload of a single endpoint, it still breaks.

I would like us to change to option 3 in the comments above, only throwing for the case that breaks. The current state needlessly breaks existing code for no real reason. The comments in option 3 about deployments being a surprise break is perfectly valid, but it's no worse than option 2 and we're seeing that break in actual runtime usage already out in the wild.

If we can't prevent the issue in general (it looks like all practical options have been exhausted there), I would argue for 2 changes:

  1. Only break when multiple endpoints are actually passed in.
  2. Provide a much clearer exception message on why we're throwing, here's a link to the current one.

Metadata

Metadata

Assignees

Type

No type

Projects

No projects

Milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions