Skip to content

Commit

Permalink
CSHARP-1736: Fix issue with .NET Core not supporting DnsEndPoint in S…
Browse files Browse the repository at this point in the history
…ocket.Connect due to bsd socket differences.
  • Loading branch information
craiggwilson committed Aug 10, 2016
1 parent ad81ac6 commit 9c2097f
Showing 1 changed file with 110 additions and 5 deletions.
115 changes: 110 additions & 5 deletions src/MongoDB.Driver.Core/Core/Connections/TcpStreamFactory.cs
Expand Up @@ -14,7 +14,9 @@
*/

using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Net;
using System.Net.Sockets;
using System.Threading;
Expand Down Expand Up @@ -46,16 +48,68 @@ public TcpStreamFactory(TcpStreamSettings settings)
// methods
public Stream CreateStream(EndPoint endPoint, CancellationToken cancellationToken)
{
#if NET45
var socket = CreateSocket(endPoint);
Connect(socket, endPoint, cancellationToken);
return CreateNetworkStream(socket);
#else
// ugh... I know... but there isn't a non-async version of dns resolution
// in .NET Core
var resolved = ResolveEndPointsAsync(endPoint).GetAwaiter().GetResult();

This comment has been minimized.

Copy link
@dot-i

dot-i Aug 16, 2016

Wouldn't be better to add ConfigureAwait(false) to avoid potential deadlock issues?

This comment has been minimized.

Copy link
@craiggwilson

craiggwilson Aug 16, 2016

Author Contributor

Since we aren't awaiting the task, and rather doing this synchronously, that wouldn't have any effect. If you have evidence of the opposite, we'd be happy to consider it.

This comment has been minimized.

Copy link
@dot-i

dot-i Aug 16, 2016

Well, I've run into this myself, and not using ConfigureAwait(false) in lower-level methods can cause two tasks to deadlock since they'll both be waiting for the same context restore to happen. Basically the rule as I understand and experienced it is: use async all the way, or use ConfigureAwait(false) if you mix sync and async.

BTW: I noticed that all other code in this TcpStreamFactory.cs file uses ConfigureAwait(false) correctly actually.

See amongst other things this for reference: http://www.tugberkugurlu.com/archive/the-perfect-recipe-to-shoot-yourself-in-the-foot-ending-up-with-a-deadlock-using-the-c-sharp-5-0-asynchronous-language-features

This comment has been minimized.

Copy link
@dot-i

dot-i Aug 16, 2016

I think your point is: the other code actually does call await, and the code above doesn't... This may make a difference, can't say for sure either way. Still think it's worth some closer investigation to make 100% sure though? :)

This comment has been minimized.

Copy link
@craiggwilson

craiggwilson Aug 16, 2016

Author Contributor

Sort of. My point is that ConfigureAwait doesn't do anything when running in a synchronous call. ConfigureAwait is mostly an instruction to the compiler to not require resumption in the same synchronization context once the task has completed. Since this is synchronous, it doesn't matter.

This comment has been minimized.

Copy link
@dot-i

dot-i Aug 16, 2016

Agreed.

for (int i = 0; i < resolved.Length; i++)
{
try
{
var socket = CreateSocket(resolved[i]);
Connect(socket, resolved[i], cancellationToken);
return CreateNetworkStream(socket);
}
catch
{
// if we have tried all of them and still failed,
// then blow up.
if (i == resolved.Length - 1)
{
throw;
}
}
}

// we should never get here...
throw new InvalidOperationException("Unabled to resolve endpoint.");
#endif
}

public async Task<Stream> CreateStreamAsync(EndPoint endPoint, CancellationToken cancellationToken)
{
#if NET45
var socket = CreateSocket(endPoint);
await ConnectAsync(socket, endPoint, cancellationToken).ConfigureAwait(false);
return CreateNetworkStream(socket);
#else
var resolved = await ResolveEndPointsAsync(endPoint).ConfigureAwait(false);
for (int i = 0; i < resolved.Length; i++)
{
try
{
var socket = CreateSocket(resolved[i]);
await ConnectAsync(socket, resolved[i], cancellationToken).ConfigureAwait(false);
return CreateNetworkStream(socket);
}
catch
{
// if we have tried all of them and still failed,
// then blow up.
if (i == resolved.Length - 1)
{
throw;
}
}
}

// we should never get here...
throw new InvalidOperationException("Unabled to resolve endpoint.");
#endif
}

// non-public methods
Expand All @@ -65,11 +119,7 @@ private void ConfigureConnectedSocket(Socket socket)
socket.ReceiveBufferSize = _settings.ReceiveBufferSize;
socket.SendBufferSize = _settings.SendBufferSize;

var socketConfigurator = _settings.SocketConfigurator;
if (socketConfigurator != null)
{
socketConfigurator(socket);
}
_settings.SocketConfigurator?.Invoke(socket);
}

private void Connect(Socket socket, EndPoint endPoint, CancellationToken cancellationToken)
Expand Down Expand Up @@ -202,7 +252,62 @@ private Socket CreateSocket(EndPoint endPoint)
{
addressFamily = _settings.AddressFamily;
}

return new Socket(addressFamily, SocketType.Stream, ProtocolType.Tcp);
}

private async Task<EndPoint[]> ResolveEndPointsAsync(EndPoint initial)
{
var dnsInitial = initial as DnsEndPoint;
if (dnsInitial == null)
{
return new[] { initial };
}

IPAddress address;
if (IPAddress.TryParse(dnsInitial.Host, out address))
{
return new[] { new IPEndPoint(address, dnsInitial.Port) };
}

var preferred = initial.AddressFamily;
if (preferred == AddressFamily.Unspecified || preferred == AddressFamily.Unknown)
{
preferred = _settings.AddressFamily;
}

return (await Dns.GetHostAddressesAsync(dnsInitial.Host).ConfigureAwait(false))
.Select(x => new IPEndPoint(x, dnsInitial.Port))
.OrderBy(x => x, new PreferredAddressFamilyComparer(preferred))
.ToArray();
}

private class PreferredAddressFamilyComparer : IComparer<EndPoint>
{
private readonly AddressFamily _preferred;

public PreferredAddressFamilyComparer(AddressFamily preferred)
{
_preferred = preferred;
}

public int Compare(EndPoint x, EndPoint y)
{
if (x.AddressFamily == y.AddressFamily)
{
return 0;
}
if (x.AddressFamily == _preferred)
{
return -1;
}
else if (y.AddressFamily == _preferred)
{
return 1;
}

return 0;
}
}
}
}

2 comments on commit 9c2097f

@CarlosTorrecillas
Copy link

Choose a reason for hiding this comment

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

would this resolve the following issue: http://stackoverflow.com/questions/38772459/connection-to-mongo-db-using-dotnetcore/38772489?noredirect=1#comment65244203_38772489 ? I raised an issue in JIRA: https://jira.mongodb.org/browse/CSHARP-1746 but then you can close it if that's a duplicate.

@craiggwilson
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I've closed as a duplicate already.

Please sign in to comment.