From 6cad2083bf694f87912c99b9e7f9950da469f970 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Foidl?= Date: Wed, 16 Dec 2020 21:24:52 +0100 Subject: [PATCH 01/11] Add cancellation support on the native side --- .../Unix/System.Native/pal_networking.c | 20 ++++++++++++++++++- .../Unix/System.Native/pal_networking.h | 19 ++++++++++-------- 2 files changed, 30 insertions(+), 9 deletions(-) diff --git a/src/libraries/Native/Unix/System.Native/pal_networking.c b/src/libraries/Native/Unix/System.Native/pal_networking.c index 5e0f37635d3264..1ad4097ca0cbff 100644 --- a/src/libraries/Native/Unix/System.Native/pal_networking.c +++ b/src/libraries/Native/Unix/System.Native/pal_networking.c @@ -310,6 +310,10 @@ static int32_t ConvertGetAddrInfoAndGetNameInfoErrorsToPal(int32_t error) case EAI_NODATA: #endif return GetAddrInfoErrorFlags_EAI_NONAME; +#if HAVE_GETADDRINFO_A + case EAI_CANCELED: + return GetAddrInfoErrorFlags_EAI_CANCELED; +#endif } assert_err(0, "Unknown AddrInfo error flag", error); @@ -586,7 +590,11 @@ int32_t SystemNative_GetHostEntryForName(const uint8_t* address, int32_t address return GetHostEntries(address, info, entry); } -int32_t SystemNative_GetHostEntryForNameAsync(const uint8_t* address, int32_t addressFamily, HostEntry* entry, GetHostEntryForNameCallback callback) +int32_t SystemNative_GetHostEntryForNameAsync(const uint8_t* address, + int32_t addressFamily, + HostEntry* entry, + GetHostEntryForNameCallback callback, + void** cancelHandle) { #if HAVE_GETADDRINFO_A if (address == NULL || entry == NULL) @@ -651,6 +659,7 @@ int32_t SystemNative_GetHostEntryForNameAsync(const uint8_t* address, int32_t ad return ConvertGetAddrInfoAndGetNameInfoErrorsToPal(result); } + *cancelHandle = &state->gai_request; return result; #else (void)address; @@ -676,6 +685,15 @@ void SystemNative_FreeHostEntry(HostEntry* entry) } } +void SystemNative_CancelGetHostEntryForNameAsync(void* cancelHandle) +{ +#if HAVE_GETADDRINFO_A + gai_cancel((struct gaicb*)cancelHandle); +#else + (void)cancelHandle; +#endif +} + // There were several versions of glibc that had the flags parameter of getnameinfo unsigned #if HAVE_GETNAMEINFO_SIGNED_FLAGS typedef int32_t NativeFlagsType; diff --git a/src/libraries/Native/Unix/System.Native/pal_networking.h b/src/libraries/Native/Unix/System.Native/pal_networking.h index 69d0d1759bb5d8..bfd99b5edd0b7c 100644 --- a/src/libraries/Native/Unix/System.Native/pal_networking.h +++ b/src/libraries/Native/Unix/System.Native/pal_networking.h @@ -23,6 +23,7 @@ typedef enum GetAddrInfoErrorFlags_EAI_BADARG = 6, // One or more input arguments were invalid. GetAddrInfoErrorFlags_EAI_NOMORE = 7, // No more entries are present in the list. GetAddrInfoErrorFlags_EAI_MEMORY = 8, // Out of memory. + GetAddrInfoErrorFlags_EAI_CANCELED = 9 // Async name resolution canceled / removed from queue } GetAddrInfoErrorFlags; /** @@ -309,19 +310,21 @@ typedef void (*GetHostEntryForNameCallback)(HostEntry* entry, int status); PALEXPORT int32_t SystemNative_GetHostEntryForNameAsync(const uint8_t* address, int32_t addressFamily, HostEntry* entry, - GetHostEntryForNameCallback callback); + GetHostEntryForNameCallback callback, + void** cancelHandle); PALEXPORT void SystemNative_FreeHostEntry(HostEntry* entry); +PALEXPORT void SystemNative_CancelGetHostEntryForNameAsync(void* cancelHandle); PALEXPORT int32_t SystemNative_GetNameInfo(const uint8_t* address, - int32_t addressLength, - int8_t isIPv6, - uint8_t* host, - int32_t hostLength, - uint8_t* service, - int32_t serviceLength, - int32_t flags); + int32_t addressLength, + int8_t isIPv6, + uint8_t* host, + int32_t hostLength, + uint8_t* service, + int32_t serviceLength, + int32_t flags); PALEXPORT int32_t SystemNative_GetDomainName(uint8_t* name, int32_t nameLength); From 76338aa47bef23ea5473ec6ad0e9f0e7f666d631 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Foidl?= Date: Wed, 16 Dec 2020 21:25:39 +0100 Subject: [PATCH 02/11] Exposed CancelGetHostEntryForNameAsync in the Interop --- .../src/Interop/Unix/System.Native/Interop.HostEntry.cs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/libraries/Common/src/Interop/Unix/System.Native/Interop.HostEntry.cs b/src/libraries/Common/src/Interop/Unix/System.Native/Interop.HostEntry.cs index 12c2c3941d2243..ac651978551980 100644 --- a/src/libraries/Common/src/Interop/Unix/System.Native/Interop.HostEntry.cs +++ b/src/libraries/Common/src/Interop/Unix/System.Native/Interop.HostEntry.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System; using System.Net.Sockets; using System.Runtime.InteropServices; @@ -21,6 +22,7 @@ internal enum GetAddrInfoErrorFlags : int EAI_BADARG = 6, // One or more input arguments were invalid. EAI_NOMORE = 7, // No more entries are present in the list. EAI_MEMORY = 8, // Out of memory. + EAI_CANCELED = 9 // Async name resolution canceled / removed from queue } [StructLayout(LayoutKind.Sequential)] @@ -43,9 +45,13 @@ internal static extern unsafe int GetHostEntryForNameAsync( string address, AddressFamily family, HostEntry* entry, - delegate* unmanaged callback); + delegate* unmanaged callback, + IntPtr* cancelHandle); [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_FreeHostEntry")] internal static extern unsafe void FreeHostEntry(HostEntry* entry); + + [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_CancelGetHostEntryForNameAsync")] + internal static extern void CancelGetHostEntryForNameAsync(IntPtr cancelHandle); } } From 17a8fe5a52e9406176e6b37232b53d1fa1c1f0e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Foidl?= Date: Wed, 16 Dec 2020 21:28:09 +0100 Subject: [PATCH 03/11] Managed implementation --- .../src/System/Net/NameResolutionPal.Unix.cs | 72 ++++++++++++++++--- 1 file changed, 61 insertions(+), 11 deletions(-) diff --git a/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Unix.cs b/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Unix.cs index e64924fe04cbb2..618252a142f008 100644 --- a/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Unix.cs +++ b/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Unix.cs @@ -79,7 +79,7 @@ public static unsafe SocketError TryGetAddrInfo(string name, bool justAddresses, public static string GetHostName() => Interop.Sys.GetHostName(); - public static unsafe Task? GetAddrInfoAsync(string hostName, bool justAddresses, AddressFamily addressFamily, CancellationToken _) + public static unsafe Task? GetAddrInfoAsync(string hostName, bool justAddresses, AddressFamily addressFamily, CancellationToken cancellationToken) { Debug.Assert(hostName is not null); @@ -94,7 +94,7 @@ public static unsafe SocketError TryGetAddrInfo(string name, bool justAddresses, GetHostEntryForNameState state; try { - state = new GetHostEntryForNameState(hostName, justAddresses); + state = new GetHostEntryForNameState(context, hostName, justAddresses); context->State = state.CreateHandle(); } catch @@ -103,13 +103,14 @@ public static unsafe SocketError TryGetAddrInfo(string name, bool justAddresses, throw; } - int errorCode = Interop.Sys.GetHostEntryForNameAsync(hostName, addressFamily, &context->Result, &GetHostEntryForNameCallback); + int error = Interop.Sys.GetHostEntryForNameAsync(hostName, addressFamily, &context->Result, &GetHostEntryForNameCallback, &context->CancelHandle); - if (errorCode != 0) + if (error != 0) { - ProcessResult(GetSocketErrorForNativeError(errorCode), context); + ProcessResult(error, context); } + state.RegisterForCancellation(cancellationToken); return state.Task; } @@ -119,16 +120,17 @@ private static unsafe void GetHostEntryForNameCallback(Interop.Sys.HostEntry* en // Can be casted directly to GetHostEntryForNameContext* because the HostEntry is its first field GetHostEntryForNameContext* context = (GetHostEntryForNameContext*)entry; - ProcessResult(GetSocketErrorForNativeError(error), context); + ProcessResult(error, context); } - private static unsafe void ProcessResult(SocketError errorCode, GetHostEntryForNameContext* context) + private static unsafe void ProcessResult(int error, GetHostEntryForNameContext* context) { try { GetHostEntryForNameState state = GetHostEntryForNameState.FromHandleAndFree(context->State); + CancellationToken cancellationToken = state.UnregisterAndGetCancellationToken(); - if (errorCode == SocketError.Success) + if (error == 0) { ParseHostEntry(context->Result, state.JustAddresses, out string? hostName, out string[] aliases, out IPAddress[] addresses); @@ -143,7 +145,10 @@ private static unsafe void ProcessResult(SocketError errorCode, GetHostEntryForN } else { - Exception ex = new SocketException((int)errorCode); + Exception ex = error == (int)Interop.Sys.GetAddrInfoErrorFlags.EAI_CANCELED && cancellationToken.IsCancellationRequested + ? (Exception)new OperationCanceledException(cancellationToken) + : new SocketException((int)GetSocketErrorForNativeError(error)); + state.SetResult(ExceptionDispatchInfo.SetCurrentStackTrace(ex)); } } @@ -251,8 +256,11 @@ private static unsafe void ParseHostEntry(Interop.Sys.HostEntry hostEntry, bool } } - private sealed class GetHostEntryForNameState : IThreadPoolWorkItem + private sealed unsafe class GetHostEntryForNameState : IThreadPoolWorkItem { + private GetHostEntryForNameContext* _cancellationContext; + private CancellationTokenRegistration _cancellationTokenRegistration; + private AsyncTaskMethodBuilder _ipAddressArrayBuilder; private AsyncTaskMethodBuilder _ipHostEntryBuilder; private object? _result; @@ -260,8 +268,9 @@ private sealed class GetHostEntryForNameState : IThreadPoolWorkItem public string HostName { get; } public bool JustAddresses { get; } - public GetHostEntryForNameState(string hostName, bool justAddresses) + public GetHostEntryForNameState(GetHostEntryForNameContext* context, string hostName, bool justAddresses) { + _cancellationContext = context; HostName = hostName; JustAddresses = justAddresses; @@ -279,6 +288,46 @@ public GetHostEntryForNameState(string hostName, bool justAddresses) public Task Task => JustAddresses ? _ipAddressArrayBuilder.Task : _ipHostEntryBuilder.Task; + public void RegisterForCancellation(CancellationToken cancellationToken) + { + if (!cancellationToken.CanBeCanceled) return; + + lock (this) + { + if (_cancellationContext is null) + { + // The operation completed before registration could be done. + return; + } + + _cancellationTokenRegistration = cancellationToken.UnsafeRegister(o => + { + var self = (GetHostEntryForNameState)o!; + + lock (self) + { + GetHostEntryForNameContext* context = self._cancellationContext; + + if (context is not null) + { + Interop.Sys.CancelGetHostEntryForNameAsync(context->CancelHandle); + } + } + }, this); + } + } + + public CancellationToken UnregisterAndGetCancellationToken() + { + lock (this) + { + _cancellationContext = null; + _cancellationTokenRegistration.Unregister(); + } + + return _cancellationTokenRegistration.Token; + } + public void SetResult(object result) { // Store the result and then queue this object to the thread pool to actually complete the Tasks, as we @@ -333,6 +382,7 @@ private unsafe struct GetHostEntryForNameContext { public Interop.Sys.HostEntry Result; public IntPtr State; + public IntPtr CancelHandle; public static GetHostEntryForNameContext* AllocateContext() { From c9e8bb8a43f00779a48bbe634b1a921df987274a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Foidl?= Date: Fri, 18 Dec 2020 13:58:43 +0100 Subject: [PATCH 04/11] Enable cancellation tests on Unix too --- .../tests/FunctionalTests/GetHostAddressesTest.cs | 1 - .../tests/FunctionalTests/GetHostEntryTest.cs | 1 - 2 files changed, 2 deletions(-) diff --git a/src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostAddressesTest.cs b/src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostAddressesTest.cs index 450357c57a130b..2eaf2456b9c502 100644 --- a/src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostAddressesTest.cs +++ b/src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostAddressesTest.cs @@ -174,7 +174,6 @@ public async Task DnsGetHostAddresses_PreCancelledToken_Throws() [OuterLoop] [Fact] [ActiveIssue("https://github.com/dotnet/runtime/issues/43816")] // Race condition outlined below. - [ActiveIssue("https://github.com/dotnet/runtime/issues/33378", TestPlatforms.AnyUnix)] // Cancellation of an outstanding getaddrinfo is not supported on *nix. public async Task DnsGetHostAddresses_PostCancelledToken_Throws() { using var cts = new CancellationTokenSource(); diff --git a/src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostEntryTest.cs b/src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostEntryTest.cs index 5356faf06467dc..fbbefc687d269a 100644 --- a/src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostEntryTest.cs +++ b/src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostEntryTest.cs @@ -264,7 +264,6 @@ public async Task DnsGetHostEntry_PreCancelledToken_Throws() [OuterLoop] [ActiveIssue("https://github.com/dotnet/runtime/issues/43816")] // Race condition outlined below. - [ActiveIssue("https://github.com/dotnet/runtime/issues/33378", TestPlatforms.AnyUnix)] // Cancellation of an outstanding getaddrinfo is not supported on *nix. [Fact] public async Task DnsGetHostEntry_PostCancelledToken_Throws() { From c4dd48e82ebd18dc89072b84856b218ba9acf961 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Foidl?= Date: Fri, 18 Dec 2020 14:32:49 +0100 Subject: [PATCH 05/11] Name resolution cancellation tests with flooding the lookup queue --- .../FunctionalTests/GetHostAddressesTest.cs | 27 +++++++++++++---- .../tests/FunctionalTests/GetHostEntryTest.cs | 29 ++++++++++++++----- 2 files changed, 43 insertions(+), 13 deletions(-) diff --git a/src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostAddressesTest.cs b/src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostAddressesTest.cs index 2eaf2456b9c502..57bdffca8e958e 100644 --- a/src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostAddressesTest.cs +++ b/src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostAddressesTest.cs @@ -176,16 +176,31 @@ public async Task DnsGetHostAddresses_PreCancelledToken_Throws() [ActiveIssue("https://github.com/dotnet/runtime/issues/43816")] // Race condition outlined below. public async Task DnsGetHostAddresses_PostCancelledToken_Throws() { - using var cts = new CancellationTokenSource(); + const int numberOfRequests = 100; + + using CancellationTokenSource cts = new(); + List tasks = new(capacity: numberOfRequests); - Task task = Dns.GetHostAddressesAsync(TestSettings.UncachedHost, cts.Token); + for (int i = 0; i < numberOfRequests; ++i) + { + Task task = Dns.GetHostAddressesAsync(TestSettings.UncachedHost, cts.Token); + tasks.Add(task); + } - // This test might flake if the cancellation token takes too long to trigger: - // It's a race between the DNS server getting back to us and the cancellation processing. cts.Cancel(); - OperationCanceledException oce = await Assert.ThrowsAnyAsync(() => task); - Assert.Equal(cts.Token, oce.CancellationToken); + await Assert.ThrowsAnyAsync(async () => + { + for (int i = 0; i < numberOfRequests; ++i) + { + try + { + await tasks[i].ConfigureAwait(false); + } + catch (Exception ex) when (ex is SocketException) + { } + } + }).ConfigureAwait(false); } } } diff --git a/src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostEntryTest.cs b/src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostEntryTest.cs index fbbefc687d269a..5f6dec07e25c7a 100644 --- a/src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostEntryTest.cs +++ b/src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostEntryTest.cs @@ -263,20 +263,35 @@ public async Task DnsGetHostEntry_PreCancelledToken_Throws() } [OuterLoop] - [ActiveIssue("https://github.com/dotnet/runtime/issues/43816")] // Race condition outlined below. [Fact] + [ActiveIssue("https://github.com/dotnet/runtime/issues/43816")] // Race condition outlined below. public async Task DnsGetHostEntry_PostCancelledToken_Throws() { - using var cts = new CancellationTokenSource(); + const int numberOfRequests = 100; - Task task = Dns.GetHostEntryAsync(TestSettings.UncachedHost, cts.Token); + using CancellationTokenSource cts = new(); + List tasks = new(capacity: numberOfRequests); + + for (int i = 0; i < numberOfRequests; ++i) + { + Task task = Dns.GetHostEntryAsync(TestSettings.UncachedHost, cts.Token); + tasks.Add(task); + } - // This test might flake if the cancellation token takes too long to trigger: - // It's a race between the DNS server getting back to us and the cancellation processing. cts.Cancel(); - OperationCanceledException oce = await Assert.ThrowsAnyAsync(() => task); - Assert.Equal(cts.Token, oce.CancellationToken); + await Assert.ThrowsAnyAsync(async () => + { + for (int i = 0; i < numberOfRequests; ++i) + { + try + { + await tasks[i].ConfigureAwait(false); + } + catch (Exception ex) when (ex is SocketException) + { } + } + }).ConfigureAwait(false); } } } From ba0aa9982b9e96636a8120a2734064aeef85fca3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Foidl?= Date: Fri, 8 Jan 2021 09:25:35 +0100 Subject: [PATCH 06/11] Update entrypoints.c --- src/libraries/Native/Unix/System.Native/entrypoints.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libraries/Native/Unix/System.Native/entrypoints.c b/src/libraries/Native/Unix/System.Native/entrypoints.c index 309eef0c8643fa..4a122c3af8ffa9 100644 --- a/src/libraries/Native/Unix/System.Native/entrypoints.c +++ b/src/libraries/Native/Unix/System.Native/entrypoints.c @@ -119,6 +119,7 @@ static const Entry s_sysNative[] = DllImportEntry(SystemNative_GetHostEntryForName) DllImportEntry(SystemNative_GetHostEntryForNameAsync) DllImportEntry(SystemNative_FreeHostEntry) + DllImportEntry(SystemNative_CancelGetHostEntryForNameAsync) DllImportEntry(SystemNative_GetNameInfo) DllImportEntry(SystemNative_GetDomainName) DllImportEntry(SystemNative_GetHostName) From bb6d07166c4480a3f6ee74577eec5d41df49eb53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Foidl?= Date: Fri, 15 Jan 2021 12:31:10 +0100 Subject: [PATCH 07/11] Fixed (forgotten) unused parameter --- src/libraries/Native/Unix/System.Native/pal_networking.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libraries/Native/Unix/System.Native/pal_networking.c b/src/libraries/Native/Unix/System.Native/pal_networking.c index 1ad4097ca0cbff..d041efd12cb0c3 100644 --- a/src/libraries/Native/Unix/System.Native/pal_networking.c +++ b/src/libraries/Native/Unix/System.Native/pal_networking.c @@ -666,6 +666,7 @@ int32_t SystemNative_GetHostEntryForNameAsync(const uint8_t* address, (void)addressFamily; (void)entry; (void)callback; + (void)cancelHandle; // GetHostEntryForNameAsync is not supported on this platform. return -1; From 7cec0f30d778162e19bbfa4a973c737da0e7766b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Foidl?= Date: Fri, 15 Jan 2021 16:57:52 +0100 Subject: [PATCH 08/11] PR Feedback --- .../Unix/System.Native/pal_networking.c | 2 +- .../src/System/Net/NameResolutionPal.Unix.cs | 16 ++++++------ .../System/Net/NameResolutionPal.Windows.cs | 2 +- .../FunctionalTests/GetHostAddressesTest.cs | 23 +++++++---------- .../tests/FunctionalTests/GetHostEntryTest.cs | 25 +++++++------------ 5 files changed, 29 insertions(+), 39 deletions(-) diff --git a/src/libraries/Native/Unix/System.Native/pal_networking.c b/src/libraries/Native/Unix/System.Native/pal_networking.c index d041efd12cb0c3..a3a7530eedd7e2 100644 --- a/src/libraries/Native/Unix/System.Native/pal_networking.c +++ b/src/libraries/Native/Unix/System.Native/pal_networking.c @@ -597,7 +597,7 @@ int32_t SystemNative_GetHostEntryForNameAsync(const uint8_t* address, void** cancelHandle) { #if HAVE_GETADDRINFO_A - if (address == NULL || entry == NULL) + if (address == NULL || entry == NULL || cancelHandle == NULL) { return GetAddrInfoErrorFlags_EAI_BADARG; } diff --git a/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Unix.cs b/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Unix.cs index 618252a142f008..cf80650cded249 100644 --- a/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Unix.cs +++ b/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Unix.cs @@ -107,7 +107,7 @@ public static unsafe SocketError TryGetAddrInfo(string name, bool justAddresses, if (error != 0) { - ProcessResult(error, context); + ProcessResult(GetSocketErrorForNativeError(error), context); } state.RegisterForCancellation(cancellationToken); @@ -120,17 +120,17 @@ private static unsafe void GetHostEntryForNameCallback(Interop.Sys.HostEntry* en // Can be casted directly to GetHostEntryForNameContext* because the HostEntry is its first field GetHostEntryForNameContext* context = (GetHostEntryForNameContext*)entry; - ProcessResult(error, context); + ProcessResult(GetSocketErrorForNativeError(error), context); } - private static unsafe void ProcessResult(int error, GetHostEntryForNameContext* context) + private static unsafe void ProcessResult(SocketError errorCode, GetHostEntryForNameContext* context) { try { GetHostEntryForNameState state = GetHostEntryForNameState.FromHandleAndFree(context->State); CancellationToken cancellationToken = state.UnregisterAndGetCancellationToken(); - if (error == 0) + if (errorCode == SocketError.Success) { ParseHostEntry(context->Result, state.JustAddresses, out string? hostName, out string[] aliases, out IPAddress[] addresses); @@ -145,9 +145,9 @@ private static unsafe void ProcessResult(int error, GetHostEntryForNameContext* } else { - Exception ex = error == (int)Interop.Sys.GetAddrInfoErrorFlags.EAI_CANCELED && cancellationToken.IsCancellationRequested + Exception ex = errorCode == SocketError.OperationAborted && cancellationToken.IsCancellationRequested ? (Exception)new OperationCanceledException(cancellationToken) - : new SocketException((int)GetSocketErrorForNativeError(error)); + : new SocketException((int)errorCode); state.SetResult(ExceptionDispatchInfo.SetCurrentStackTrace(ex)); } @@ -177,6 +177,8 @@ private static SocketError GetSocketErrorForNativeError(int error) return SocketError.HostNotFound; case (int)Interop.Sys.GetAddrInfoErrorFlags.EAI_MEMORY: throw new OutOfMemoryException(); + case (int)Interop.Sys.GetAddrInfoErrorFlags.EAI_CANCELED: + return SocketError.OperationAborted; default: Debug.Fail("Unexpected error: " + error.ToString()); return SocketError.SocketError; @@ -300,7 +302,7 @@ public void RegisterForCancellation(CancellationToken cancellationToken) return; } - _cancellationTokenRegistration = cancellationToken.UnsafeRegister(o => + _cancellationTokenRegistration = cancellationToken.UnsafeRegister(static o => { var self = (GetHostEntryForNameState)o!; diff --git a/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Windows.cs b/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Windows.cs index a27ce271b3134d..1862f1cd0adc2b 100644 --- a/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Windows.cs +++ b/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Windows.cs @@ -403,7 +403,7 @@ public void RegisterForCancellation(CancellationToken cancellationToken) return; } - _cancellationRegistration = cancellationToken.UnsafeRegister(o => + _cancellationRegistration = cancellationToken.UnsafeRegister(static o => { var @this = (GetAddrInfoExState)o!; int cancelResult = 0; diff --git a/src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostAddressesTest.cs b/src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostAddressesTest.cs index 57bdffca8e958e..2521973aa9e78b 100644 --- a/src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostAddressesTest.cs +++ b/src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostAddressesTest.cs @@ -176,12 +176,12 @@ public async Task DnsGetHostAddresses_PreCancelledToken_Throws() [ActiveIssue("https://github.com/dotnet/runtime/issues/43816")] // Race condition outlined below. public async Task DnsGetHostAddresses_PostCancelledToken_Throws() { - const int numberOfRequests = 100; + const int NumberOfRequests = 100; using CancellationTokenSource cts = new(); - List tasks = new(capacity: numberOfRequests); + List tasks = new(capacity: NumberOfRequests); - for (int i = 0; i < numberOfRequests; ++i) + for (int i = 0; i < NumberOfRequests; ++i) { Task task = Dns.GetHostAddressesAsync(TestSettings.UncachedHost, cts.Token); tasks.Add(task); @@ -189,18 +189,13 @@ public async Task DnsGetHostAddresses_PostCancelledToken_Throws() cts.Cancel(); - await Assert.ThrowsAnyAsync(async () => + try { - for (int i = 0; i < numberOfRequests; ++i) - { - try - { - await tasks[i].ConfigureAwait(false); - } - catch (Exception ex) when (ex is SocketException) - { } - } - }).ConfigureAwait(false); + await Task.WhenAll(tasks); + } + catch { } + + Assert.Contains(tasks, t => t.Exception?.InnerException is OperationCanceledException); } } } diff --git a/src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostEntryTest.cs b/src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostEntryTest.cs index 5f6dec07e25c7a..96f1bbfe304e13 100644 --- a/src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostEntryTest.cs +++ b/src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostEntryTest.cs @@ -267,31 +267,24 @@ public async Task DnsGetHostEntry_PreCancelledToken_Throws() [ActiveIssue("https://github.com/dotnet/runtime/issues/43816")] // Race condition outlined below. public async Task DnsGetHostEntry_PostCancelledToken_Throws() { - const int numberOfRequests = 100; + const int NumberOfRequests = 100; using CancellationTokenSource cts = new(); - List tasks = new(capacity: numberOfRequests); + List tasks = new(capacity: NumberOfRequests); - for (int i = 0; i < numberOfRequests; ++i) + for (int i = 0; i < NumberOfRequests; ++i) { Task task = Dns.GetHostEntryAsync(TestSettings.UncachedHost, cts.Token); tasks.Add(task); } - cts.Cancel(); - - await Assert.ThrowsAnyAsync(async () => + try { - for (int i = 0; i < numberOfRequests; ++i) - { - try - { - await tasks[i].ConfigureAwait(false); - } - catch (Exception ex) when (ex is SocketException) - { } - } - }).ConfigureAwait(false); + await Task.WhenAll(tasks); + } + catch { } + + Assert.Contains(tasks, t => t.Exception?.InnerException is OperationCanceledException); } } } From 62f05fe25098f96ccba82dc238174ed2a94f4be6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Foidl?= Date: Fri, 22 Jan 2021 15:42:02 +0100 Subject: [PATCH 09/11] Use SkipTestException Cf. https://github.com/dotnet/runtime/pull/47036#discussion_r558828713 --- .../tests/FunctionalTests/GetHostAddressesTest.cs | 5 +++-- .../tests/FunctionalTests/GetHostEntryTest.cs | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostAddressesTest.cs b/src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostAddressesTest.cs index 2521973aa9e78b..6c63594d109c8e 100644 --- a/src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostAddressesTest.cs +++ b/src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostAddressesTest.cs @@ -5,7 +5,7 @@ using System.Net.Sockets; using System.Threading; using System.Threading.Tasks; - +using Microsoft.DotNet.XUnitExtensions; using Xunit; namespace System.Net.NameResolution.Tests @@ -172,7 +172,7 @@ public async Task DnsGetHostAddresses_PreCancelledToken_Throws() } [OuterLoop] - [Fact] + [ConditionalFact] [ActiveIssue("https://github.com/dotnet/runtime/issues/43816")] // Race condition outlined below. public async Task DnsGetHostAddresses_PostCancelledToken_Throws() { @@ -192,6 +192,7 @@ public async Task DnsGetHostAddresses_PostCancelledToken_Throws() try { await Task.WhenAll(tasks); + throw new SkipTestException("GetHostAddressesAsync should fail but it did not."); } catch { } diff --git a/src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostEntryTest.cs b/src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostEntryTest.cs index 96f1bbfe304e13..92df7fd1eea874 100644 --- a/src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostEntryTest.cs +++ b/src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostEntryTest.cs @@ -6,7 +6,7 @@ using System.Net.Sockets; using System.Threading; using System.Threading.Tasks; - +using Microsoft.DotNet.XUnitExtensions; using Xunit; namespace System.Net.NameResolution.Tests @@ -263,7 +263,7 @@ public async Task DnsGetHostEntry_PreCancelledToken_Throws() } [OuterLoop] - [Fact] + [ConditionalFact] [ActiveIssue("https://github.com/dotnet/runtime/issues/43816")] // Race condition outlined below. public async Task DnsGetHostEntry_PostCancelledToken_Throws() { @@ -281,6 +281,7 @@ public async Task DnsGetHostEntry_PostCancelledToken_Throws() try { await Task.WhenAll(tasks); + throw new SkipTestException("GetHostEntryAsync should fail but it did not."); } catch { } From d0fd3e0773c8a5703ffeda9e1b118e0d56f74210 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Foidl?= Date: Wed, 3 Feb 2021 17:41:23 +0100 Subject: [PATCH 10/11] PR Feedback nits --- .../src/System/Net/NameResolutionPal.Unix.cs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Unix.cs b/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Unix.cs index cf80650cded249..980df99d65fc07 100644 --- a/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Unix.cs +++ b/src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Unix.cs @@ -292,7 +292,10 @@ public GetHostEntryForNameState(GetHostEntryForNameContext* context, string host public void RegisterForCancellation(CancellationToken cancellationToken) { - if (!cancellationToken.CanBeCanceled) return; + if (!cancellationToken.CanBeCanceled) + { + return; + } lock (this) { @@ -305,11 +308,9 @@ public void RegisterForCancellation(CancellationToken cancellationToken) _cancellationTokenRegistration = cancellationToken.UnsafeRegister(static o => { var self = (GetHostEntryForNameState)o!; - lock (self) { GetHostEntryForNameContext* context = self._cancellationContext; - if (context is not null) { Interop.Sys.CancelGetHostEntryForNameAsync(context->CancelHandle); From a148365d8ad1dff4a06e1a28bf29b8a426ad003c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Foidl?= Date: Wed, 3 Feb 2021 17:53:28 +0100 Subject: [PATCH 11/11] Tests with retry until cancellation succeeds or the test hangs Cf. https://github.com/dotnet/runtime/pull/47036#discussion_r569527842 --- .../FunctionalTests/GetHostAddressesTest.cs | 43 +++++++++++-------- .../tests/FunctionalTests/GetHostEntryTest.cs | 39 +++++++++++------ 2 files changed, 51 insertions(+), 31 deletions(-) diff --git a/src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostAddressesTest.cs b/src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostAddressesTest.cs index 6c63594d109c8e..6d2a0403633667 100644 --- a/src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostAddressesTest.cs +++ b/src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostAddressesTest.cs @@ -178,25 +178,34 @@ public async Task DnsGetHostAddresses_PostCancelledToken_Throws() { const int NumberOfRequests = 100; - using CancellationTokenSource cts = new(); - List tasks = new(capacity: NumberOfRequests); - - for (int i = 0; i < NumberOfRequests; ++i) + // Retry until it is canceled, to notice hangs if cancellation doesn't work. + while (true) { - Task task = Dns.GetHostAddressesAsync(TestSettings.UncachedHost, cts.Token); - tasks.Add(task); + using CancellationTokenSource cts = new(); + List tasks = new(capacity: NumberOfRequests); + + for (int i = 0; i < NumberOfRequests; ++i) + { + Task task = Dns.GetHostAddressesAsync(TestSettings.UncachedHost, cts.Token); + tasks.Add(task); + } + + cts.Cancel(); + + try + { + await Task.WhenAll(tasks); + } + catch { } + + foreach (Task task in tasks) + { + if (task.Exception?.InnerException is OperationCanceledException) + { + return; + } + } } - - cts.Cancel(); - - try - { - await Task.WhenAll(tasks); - throw new SkipTestException("GetHostAddressesAsync should fail but it did not."); - } - catch { } - - Assert.Contains(tasks, t => t.Exception?.InnerException is OperationCanceledException); } } } diff --git a/src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostEntryTest.cs b/src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostEntryTest.cs index 92df7fd1eea874..9524f38a78c988 100644 --- a/src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostEntryTest.cs +++ b/src/libraries/System.Net.NameResolution/tests/FunctionalTests/GetHostEntryTest.cs @@ -269,23 +269,34 @@ public async Task DnsGetHostEntry_PostCancelledToken_Throws() { const int NumberOfRequests = 100; - using CancellationTokenSource cts = new(); - List tasks = new(capacity: NumberOfRequests); - - for (int i = 0; i < NumberOfRequests; ++i) + // Retry until it is canceled, to notice hangs if cancellation doesn't work. + while (true) { - Task task = Dns.GetHostEntryAsync(TestSettings.UncachedHost, cts.Token); - tasks.Add(task); - } + using CancellationTokenSource cts = new(); + List tasks = new(capacity: NumberOfRequests); - try - { - await Task.WhenAll(tasks); - throw new SkipTestException("GetHostEntryAsync should fail but it did not."); - } - catch { } + for (int i = 0; i < NumberOfRequests; ++i) + { + Task task = Dns.GetHostEntryAsync(TestSettings.UncachedHost, cts.Token); + tasks.Add(task); + } - Assert.Contains(tasks, t => t.Exception?.InnerException is OperationCanceledException); + cts.Cancel(); + + try + { + await Task.WhenAll(tasks); + } + catch { } + + foreach (Task task in tasks) + { + if (task.Exception?.InnerException is OperationCanceledException) + { + return; + } + } + } } } }