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

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 (file)
--- 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<PingReply> SendPingAsyncCore(IPAddress address, byte[] buffer, int timeout, PingOptions options)
         {
-            try
-            {
-                Task<PingReply> t = RawSocketPermissions.CanUseRawSockets(address.AddressFamily) ?
+            Task<PingReply> 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 (file)
--- 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<PingReply> 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 (file)
--- 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<PingReply> SendPingAsync(IPAddress address, int timeout, byte[] buffer, PingOptions options)
         {
             CheckArgs(address, timeout, buffer, options);
+            return SendPingAsyncInternal(address, timeout, buffer, options);
+        }
 
+        private async Task<PingReply> 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<PingReply> 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<PingReply>(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<PingReply> GetAddressAndSendAsync(string hostNameOrAddress, int timeout, byte[] buffer, PingOptions options)
         {
-            bool requiresFinish = true;
+            CheckStart();
             try
             {
                 IPAddress[] addresses = await Dns.GetHostAddressesAsync(hostNameOrAddress).ConfigureAwait(false);
                 Task<PingReply> 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.