-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Modified Dns.GetHostAddressesAsync to be truly async #26850
Changes from 1 commit
e31d807
77ccb1c
a6cff67
898719f
c173a88
a375f91
795e5be
a9a3403
91864e5
524b5fa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
using System.Net.Internals; | ||
using System.Runtime.InteropServices; | ||
|
||
namespace System.Net.Sockets | ||
{ | ||
[StructLayout(LayoutKind.Sequential)] | ||
internal unsafe struct AddressInfoEx | ||
{ | ||
internal AddressInfoHints ai_flags; | ||
internal AddressFamily ai_family; | ||
internal SocketType ai_socktype; | ||
internal ProtocolFamily ai_protocol; | ||
internal int ai_addrlen; | ||
internal IntPtr ai_canonname; // Ptr to the canonical name - check for NULL | ||
internal byte* ai_addr; // Ptr to the sockaddr structure | ||
internal IntPtr ai_blob; // Unused ptr to blob data about provider | ||
internal int ai_bloblen; | ||
internal IntPtr ai_provider; // Unused ptr to the namespace provider guid | ||
internal AddressInfoEx* ai_next; // Next structure in linked list | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
using System; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing license header |
||
using System.Net.Sockets; | ||
using System.Runtime.ConstrainedExecution; | ||
using System.Runtime.InteropServices; | ||
using System.Threading; | ||
|
||
internal static partial class Interop | ||
{ | ||
internal static partial class Winsock | ||
{ | ||
internal unsafe delegate void GetAddrInfoExCompletionCallback([In] int error, [In] int bytes, [In] NativeOverlapped* overlapped); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: |
||
|
||
[DllImport(Interop.Libraries.Ws2_32, ExactSpelling = true, CharSet = CharSet.Unicode, BestFitMapping = false, ThrowOnUnmappableChar = true, SetLastError = true)] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I copied theses properties from the getaddrinfo declaration without thinking. It doesn't make sense since the call is Unicode, I will remove ThrowOnUnmappableChar and BestFitMapping properties. |
||
internal static extern unsafe int GetAddrInfoExW( | ||
[In] string name, | ||
[In] string serviceName, | ||
[In] int namespaceId, | ||
[In] IntPtr providerId, | ||
[In] ref AddressInfoEx hints, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The build is passing but Visual Studio is complaining with "Feature 'readonly references' is not available in C# 7.0. Please use language version 7.2 or greater." I suggest leaving 'ref', I don't think it would change anything for a p/invoke call anyway. |
||
[Out] out AddressInfoEx* result, | ||
[In] IntPtr timeout, | ||
[In] ref NativeOverlapped overlapped, | ||
[In] GetAddrInfoExCompletionCallback callback, | ||
[Out] out IntPtr cancelHandle | ||
); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: We try to use the same names as in the native definition, so e.g. |
||
|
||
[ReliabilityContract(Consistency.WillNotCorruptState, Cer.Success)] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These ReliabilityContract attributes don't have a meaning in .NET Core and can be removed. |
||
[DllImport("ws2_32.dll", ExactSpelling = true, SetLastError = true)] | ||
internal static extern unsafe void FreeAddrInfoEx([In] AddressInfoEx* addressInfo); | ||
|
||
[ReliabilityContract(Consistency.WillNotCorruptState, Cer.Success)] | ||
[DllImport("ws2_32.dll", ExactSpelling = true, SetLastError = true)] | ||
internal static extern int GetAddrInfoExCancel([In] ref IntPtr cancelHandle); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same ref => in question. |
||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -252,32 +252,9 @@ public static IPHostEntry Resolve(string hostName) | |
return ipHostEntry; | ||
} | ||
|
||
private class ResolveAsyncResult : ContextAwareResult | ||
{ | ||
// Forward lookup | ||
internal ResolveAsyncResult(string hostName, object myObject, bool includeIPv6, object myState, AsyncCallback myCallBack) : | ||
base(myObject, myState, myCallBack) | ||
{ | ||
this.hostName = hostName; | ||
this.includeIPv6 = includeIPv6; | ||
} | ||
|
||
// Reverse lookup | ||
internal ResolveAsyncResult(IPAddress address, object myObject, bool includeIPv6, object myState, AsyncCallback myCallBack) : | ||
base(myObject, myState, myCallBack) | ||
{ | ||
this.includeIPv6 = includeIPv6; | ||
this.address = address; | ||
} | ||
|
||
internal readonly string hostName; | ||
internal bool includeIPv6; | ||
internal IPAddress address; | ||
} | ||
|
||
private static void ResolveCallback(object context) | ||
{ | ||
ResolveAsyncResult result = (ResolveAsyncResult)context; | ||
DnsResolveAsyncResult result = (DnsResolveAsyncResult)context; | ||
IPHostEntry hostEntry; | ||
try | ||
{ | ||
|
@@ -316,15 +293,15 @@ private static IAsyncResult HostResolutionBeginHelper(string hostName, bool just | |
|
||
// See if it's an IP Address. | ||
IPAddress address; | ||
ResolveAsyncResult asyncResult; | ||
DnsResolveAsyncResult asyncResult; | ||
if (IPAddress.TryParse(hostName, out address)) | ||
{ | ||
if (throwOnIIPAny && (address.Equals(IPAddress.Any) || address.Equals(IPAddress.IPv6Any))) | ||
{ | ||
throw new ArgumentException(SR.net_invalid_ip_addr, nameof(hostName)); | ||
} | ||
|
||
asyncResult = new ResolveAsyncResult(address, null, includeIPv6, state, requestCallback); | ||
asyncResult = new DnsResolveAsyncResult(address, null, includeIPv6, state, requestCallback); | ||
|
||
if (justReturnParsedIp) | ||
{ | ||
|
@@ -337,19 +314,26 @@ private static IAsyncResult HostResolutionBeginHelper(string hostName, bool just | |
} | ||
else | ||
{ | ||
asyncResult = new ResolveAsyncResult(hostName, null, includeIPv6, state, requestCallback); | ||
asyncResult = new DnsResolveAsyncResult(hostName, null, includeIPv6, state, requestCallback); | ||
} | ||
|
||
// Set up the context, possibly flow. | ||
asyncResult.StartPostingAsyncOp(false); | ||
|
||
// Start the resolve. | ||
Task.Factory.StartNew( | ||
s => ResolveCallback(s), | ||
asyncResult, | ||
CancellationToken.None, | ||
TaskCreationOptions.DenyChildAttach, | ||
TaskScheduler.Default); | ||
if (NameResolutionPal.SupportsGetAddrInfoAsync && includeIPv6 && SocketProtocolSupportPal.OSSupportsIPv6 && address == null) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. More generally, why is OSSupportsIPv6 necessary for SupportsGetAddrInfoAsync? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. GetAddrInfoEx doesn't require the OS to support IPv6. But the previous implementation was using GetHostByName instead of GetAddrInfo is OSSupportsIPv6 is false. I'm not sure why that choice was made originally, but I thought it safe to not change this behavior. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just saw the explanation in InternalGetHostByName
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that comment is ancient. There's no reason for us to not always use getaddrinfo today, and it would simplify the code and the tests. My concern is mainly that we not make things any worse than they currently are. So if you want to do the IPv6 checks, that's fine, but please add comments wherever you are doing so that refer back to the comment above, so it's clear why these checks are happening. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I created issue #26856 to track this. |
||
{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: can you add a comment about the conditions that let's us take this path? |
||
NameResolutionPal.GetAddrInfoAsync(hostName, asyncResult); | ||
} | ||
else | ||
{ | ||
// Start the resolve. | ||
Task.Factory.StartNew( | ||
s => ResolveCallback(s), | ||
asyncResult, | ||
CancellationToken.None, | ||
TaskCreationOptions.DenyChildAttach, | ||
TaskScheduler.Default); | ||
} | ||
|
||
// Finish the flowing, maybe it completed? This does nothing if we didn't initiate the flowing above. | ||
asyncResult.FinishPostingAsyncOp(); | ||
|
@@ -371,7 +355,7 @@ private static IAsyncResult HostResolutionBeginHelper(IPAddress address, bool fl | |
if (NetEventSource.IsEnabled) NetEventSource.Info(null, address); | ||
|
||
// Set up the context, possibly flow. | ||
ResolveAsyncResult asyncResult = new ResolveAsyncResult(address, null, includeIPv6, state, requestCallback); | ||
DnsResolveAsyncResult asyncResult = new DnsResolveAsyncResult(address, null, includeIPv6, state, requestCallback); | ||
if (flowContext) | ||
{ | ||
asyncResult.StartPostingAsyncOp(false); | ||
|
@@ -399,7 +383,7 @@ private static IPHostEntry HostResolutionEndHelper(IAsyncResult asyncResult) | |
{ | ||
throw new ArgumentNullException(nameof(asyncResult)); | ||
} | ||
ResolveAsyncResult castedResult = asyncResult as ResolveAsyncResult; | ||
DnsResolveAsyncResult castedResult = asyncResult as DnsResolveAsyncResult; | ||
if (castedResult == null) | ||
{ | ||
throw new ArgumentException(SR.net_io_invalidasyncresult, nameof(asyncResult)); | ||
|
@@ -611,7 +595,7 @@ public static IPHostEntry EndResolve(IAsyncResult asyncResult) | |
} | ||
catch (SocketException ex) | ||
{ | ||
IPAddress address = ((ResolveAsyncResult)asyncResult).address; | ||
IPAddress address = ((DnsResolveAsyncResult)asyncResult).address; | ||
if (address == null) | ||
throw; // BeginResolve was called with a HostName, not an IPAddress | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
namespace System.Net | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing license header There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
{ | ||
internal class DnsResolveAsyncResult : ContextAwareResult | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: sealed |
||
{ | ||
// Forward lookup | ||
internal DnsResolveAsyncResult(string hostName, object myObject, bool includeIPv6, object myState, AsyncCallback myCallBack) : | ||
base(myObject, myState, myCallBack) | ||
{ | ||
this.hostName = hostName; | ||
this.includeIPv6 = includeIPv6; | ||
} | ||
|
||
// Reverse lookup | ||
internal DnsResolveAsyncResult(IPAddress address, object myObject, bool includeIPv6, object myState, AsyncCallback myCallBack) : | ||
base(myObject, myState, myCallBack) | ||
{ | ||
this.includeIPv6 = includeIPv6; | ||
this.address = address; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: the "this"es shouldn't be necessary |
||
} | ||
|
||
internal readonly string hostName; | ||
internal bool includeIPv6; | ||
internal IPAddress address; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: these should be prefixed with _ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can these be made readonly? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was existing code moved from DNS.cs, but I will make the changes |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,8 @@ namespace System.Net | |
{ | ||
internal static partial class NameResolutionPal | ||
{ | ||
public static bool SupportsGetAddrInfoAsync => false; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you make this a const? That would help the compiler to eliminate branches at the call site when doing the unix build. (It can be const in the Unix build and a static property in the Windows one.) |
||
|
||
private static SocketError GetSocketErrorForErrno(int errno) | ||
{ | ||
switch (errno) | ||
|
@@ -194,6 +196,11 @@ public static unsafe SocketError TryGetAddrInfo(string name, out IPHostEntry hos | |
return SocketError.Success; | ||
} | ||
|
||
internal static void GetAddrInfoAsync(string name, Dns.ResolveAsyncResult asyncResult) | ||
{ | ||
throw new NotSupportedException(); | ||
} | ||
|
||
public static unsafe string TryGetNameInfo(IPAddress addr, out SocketError socketError, out int nativeErrorCode) | ||
{ | ||
byte* buffer = stackalloc byte[Interop.Sys.NI_MAXHOST + 1 /*for null*/]; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing license header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done