From d9492c6fc7ce9a802c1bfd8b97eea0f1083e7529 Mon Sep 17 00:00:00 2001 From: Tomas Weinfurt Date: Wed, 13 Mar 2019 09:17:01 -0700 Subject: [PATCH] small fixes for ping to properly set state in case of failure (dotnet/corefx#33621) * unix ping fixes * feedback from review and fixes for failing tests * feedback from review * more rework on cleanup logic * add missing check for disposed object in Async path * remove freebsd part of this change to make it cleaner * await for all calls of SendPingAsyncCore() * make arg checking synchronous and stabilize SendPingAsync_InvalidArgs failing on some platforms * feedback from review Commit migrated from https://github.com/dotnet/corefx/commit/2696d751a7b50bdc3e03bf5089f67aa0bd081b24 --- .../src/System/Net/NetworkInformation/Ping.Unix.cs | 31 ++++--------- .../System/Net/NetworkInformation/Ping.Windows.cs | 23 ++++------ .../src/System/Net/NetworkInformation/Ping.cs | 51 ++++++++++++++-------- 3 files changed, 50 insertions(+), 55 deletions(-) diff --git a/src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.Unix.cs b/src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.Unix.cs index 3a4e3ca..95c4141 100644 --- a/src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.Unix.cs +++ b/src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.Unix.cs @@ -23,39 +23,26 @@ namespace System.Net.NetworkInformation private PingReply SendPingCore(IPAddress address, byte[] buffer, int timeout, PingOptions options) { - try - { - PingReply reply = RawSocketPermissions.CanUseRawSockets(address.AddressFamily) ? + PingReply reply = RawSocketPermissions.CanUseRawSockets(address.AddressFamily) ? SendIcmpEchoRequestOverRawSocket(address, buffer, timeout, options) : SendWithPingUtility(address, buffer, timeout, options); - - return reply; - } - finally - { - Finish(); - } + return reply; } private async Task SendPingAsyncCore(IPAddress address, byte[] buffer, int timeout, PingOptions options) { - try - { - Task t = RawSocketPermissions.CanUseRawSockets(address.AddressFamily) ? + Task t = RawSocketPermissions.CanUseRawSockets(address.AddressFamily) ? SendIcmpEchoRequestOverRawSocketAsync(address, buffer, timeout, options) : SendWithPingUtilityAsync(address, buffer, timeout, options); - PingReply reply = await t.ConfigureAwait(false); - if (_canceled) - { - throw new OperationCanceledException(); - } - return reply; - } - finally + PingReply reply = await t.ConfigureAwait(false); + + if (_canceled) { - Finish(); + throw new OperationCanceledException(); } + + return reply; } private SocketConfig GetSocketConfig(IPAddress address, byte[] buffer, int timeout, PingOptions options) diff --git a/src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.Windows.cs b/src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.Windows.cs index 2634067..1591648 100644 --- a/src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.Windows.cs +++ b/src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.Windows.cs @@ -31,14 +31,14 @@ namespace System.Net.NetworkInformation private PingReply SendPingCore(IPAddress address, byte[] buffer, int timeout, PingOptions options) { - // Since isAsync == false, DoSendPingCore will execute synchronously and return a completed + // Since isAsync == false, DoSendPingCore will execute synchronously and return a completed // Task - so no blocking here - return DoSendPingCore(address, buffer, timeout, options, isAsync: false).Result; + return DoSendPingCore(address, buffer, timeout, options, isAsync: false).GetAwaiter().GetResult(); } private Task SendPingAsyncCore(IPAddress address, byte[] buffer, int timeout, PingOptions options) { - // Since isAsync == true, DoSendPingCore will execute asynchronously and return an active Task + // Since isAsync == true, DoSendPingCore will execute asynchronously and return an active Task return DoSendPingCore(address, buffer, timeout, options, isAsync: true); } @@ -77,7 +77,7 @@ namespace System.Net.NetworkInformation } catch { - Cleanup(isAsync, isComplete: false); + Cleanup(isAsync); throw; } @@ -88,7 +88,7 @@ namespace System.Net.NetworkInformation // Only skip Async IO Pending error value. if (!isAsync || error != Interop.IpHlpApi.ERROR_IO_PENDING) { - Cleanup(isAsync, isComplete: false); + Cleanup(isAsync); throw new Win32Exception(error); } } @@ -96,7 +96,7 @@ namespace System.Net.NetworkInformation if (isAsync) return tcs.Task; - Cleanup(isAsync: false, isComplete: true); + Cleanup(isAsync); return Task.FromResult(CreatePingReply()); } @@ -215,7 +215,7 @@ namespace System.Net.NetworkInformation return CreatePingReplyFromIcmpEchoReply(icmpReply); } - private void Cleanup(bool isAsync, bool isComplete) + private void Cleanup(bool isAsync) { FreeUnmanagedStructures(); @@ -223,13 +223,8 @@ namespace System.Net.NetworkInformation { UnregisterWaitHandle(); } - - if (isComplete) - { - Finish(); - } } - + partial void InternalDisposeCore() { if (_handlePingV4 != null) @@ -285,7 +280,7 @@ namespace System.Net.NetworkInformation } finally { - Cleanup(isAsync: true, isComplete: true); + Cleanup(isAsync: true); } // Once we've called Finish, complete the task diff --git a/src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.cs b/src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.cs index 3039c6d..924c432 100644 --- a/src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.cs +++ b/src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.cs @@ -41,8 +41,9 @@ namespace System.Net.NetworkInformation } } - private static void CheckArgs(int timeout, byte[] buffer, PingOptions options) + private void CheckArgs(int timeout, byte[] buffer, PingOptions options) { + CheckDisposed(); if (buffer == null) { throw new ArgumentNullException(nameof(buffer)); @@ -76,14 +77,17 @@ namespace System.Net.NetworkInformation throw new ArgumentException(SR.net_invalid_ip_addr, nameof(address)); } } - - private void CheckStart() + + private void CheckDisposed() { if (_disposeRequested) { throw new ObjectDisposedException(GetType().FullName); } + } + private void CheckStart() + { int currentStatus; lock (_lockObject) { @@ -211,7 +215,6 @@ namespace System.Net.NetworkInformation } CheckArgs(timeout, buffer, options); - CheckStart(); return GetAddressAndSend(hostNameOrAddress, timeout, buffer, options); } @@ -231,11 +234,14 @@ namespace System.Net.NetworkInformation } catch (Exception e) { - Finish(); throw new PingException(SR.net_ping, e); } + finally + { + Finish(); + } } - + public void SendAsync(string hostNameOrAddress, object userToken) { SendAsync(hostNameOrAddress, DefaultTimeout, DefaultSendBuffer, userToken); @@ -320,7 +326,11 @@ namespace System.Net.NetworkInformation public Task SendPingAsync(IPAddress address, int timeout, byte[] buffer, PingOptions options) { CheckArgs(address, timeout, buffer, options); + return SendPingAsyncInternal(address, timeout, buffer, options); + } + private async Task SendPingAsyncInternal(IPAddress address, int timeout, byte[] buffer, PingOptions options) + { // Need to snapshot the address here, so we're sure that it's not changed between now // and the operation, and to be sure that IPAddress.ToString() is called and not some override. IPAddress addressSnapshot = GetAddressSnapshot(address); @@ -328,12 +338,16 @@ namespace System.Net.NetworkInformation CheckStart(); try { - return SendPingAsyncCore(addressSnapshot, buffer, timeout, options); + Task pingReplyTask = SendPingAsyncCore(addressSnapshot, buffer, timeout, options); + return await pingReplyTask.ConfigureAwait(false); } catch (Exception e) { + throw new PingException(SR.net_ping, e); + } + finally + { Finish(); - return Task.FromException(new PingException(SR.net_ping, e)); } } @@ -350,7 +364,6 @@ namespace System.Net.NetworkInformation } CheckArgs(timeout, buffer, options); - CheckStart(); return GetAddressAndSendAsync(hostNameOrAddress, timeout, buffer, options); } @@ -374,6 +387,7 @@ namespace System.Net.NetworkInformation private PingReply GetAddressAndSend(string hostNameOrAddress, int timeout, byte[] buffer, PingOptions options) { + CheckStart(); try { IPAddress[] addresses = Dns.GetHostAddresses(hostNameOrAddress); @@ -381,32 +395,31 @@ namespace System.Net.NetworkInformation } catch (Exception e) { - Finish(); throw new PingException(SR.net_ping, e); } + finally + { + Finish(); + } } private async Task GetAddressAndSendAsync(string hostNameOrAddress, int timeout, byte[] buffer, PingOptions options) { - bool requiresFinish = true; + CheckStart(); try { IPAddress[] addresses = await Dns.GetHostAddressesAsync(hostNameOrAddress).ConfigureAwait(false); Task pingReplyTask = SendPingAsyncCore(addresses[0], buffer, timeout, options); - requiresFinish = false; return await pingReplyTask.ConfigureAwait(false); } catch (Exception e) { - // SendPingAsyncCore will call Finish before completing the Task. If SendPingAsyncCore isn't invoked - // because an exception is thrown first, or if it throws out an exception synchronously, then - // we need to invoke Finish; otherwise, it has the responsibility to invoke Finish. - if (requiresFinish) - { - Finish(); - } throw new PingException(SR.net_ping, e); } + finally + { + Finish(); + } } // Tests if the current machine supports the given ip protocol family. -- 2.7.4