Runtime: System.Net.NetworkInformation.Ping is throwing exceptions for anything but TtlExpired or Success

[vexx32]

So I ran into this originally while working on PowerShell's Test-Connection cmdlet. Below is a very simple C# repro of the bug:

using System;
using System.Net.NetworkInformation;

namespace Pings
{
    class Program
    {
        static void Main(string[] args)
        {
            try
            {
                var sender = new Ping();
                var reply = sender.Send("1.2.3.4");

                Console.WriteLine("Status {0}; Latency: {0} ms", reply.Status, reply.RoundtripTime);
            }
            catch (PingException e)
            {
                Console.WriteLine("Exception occurred: {0}", e.Message);
                Console.WriteLine("Exception HResult: {0}", e.HResult);
                Console.WriteLine("Exception stack trace:\n{0}", e.StackTrace);
                System.Environment.Exit(e.HResult);
            }
        }
    }
}

Try it with both netcoreapp2.1 and netcoreapp3.0 targets with a simple dotnet run after adding a very basic csproj.

netcoreapp2.1

netcoreapp3.0

You'll notice netcoreapp2.1 _does not throw_ an exception. The inevitable time out is handled as a time out PingReply and returned to the caller properly. This will be very similar with the async methods as well, I believe.

In netcoreapp3.0 the native exception is not properly handled and is improperly surfaced, along with HResult values and so forth. This appears to be a regression that was introduced in dotnet/corefx#30000.

The affected code is here:
https://github.com/dotnet/corefx/blob/42ecf717478504fd7cc072f3586c6edd1ba57daa/src/System.Net.Ping/src/System/Net/NetworkInformation/Ping.Windows.cs#L92

Also possibly related:

• https://stackoverflow.com/questions/23374710/icmpsendecho2-fails-with-fails-with-wsa-qos-admission-failure-and-error-noaccess
• dotnet/runtime#29803

Any chance we could sort this out for .NET Core 3 release? 💖

/cc @seeminglyscience who helped figure out the details of what was going wrong here 😘

[davidsh]

@wfurt

[wfurt]

it seems like 3.0 regression so we should fix it.
I'll look why it did slip through our tests.

[wfurt]

I did quick look and here is what is happening.
The Async() version treats ALL failures as timeout in CreatePingReplyFromIcmpEchoReply() and I verified it still does in 3.0. With dotnet/corefx#30000 we are now on separate path where we return failures from native API. And then we wrap them in PingException if SendPingCore() fails.

That seems like a better approach as it allows to distinguish API failures and failures to send request from actual timeout. The caveat is that in this case we did send ICMP request and never got answer back. Perhaps the fix may be to pay closer attention to hresult or simply go back to original behavior and treat everything as timeout.
Quick and easy fix to to flip isAsync in SendPingCore.

cc: @scalablecory

[vexx32]

@wfurt the returned exceptions from the native API are much less usable than the objects that come back from SendAsync() -- have actually switched to that pattern now and realised it was doing it differently a couple hours after filing this, so thanks for the explanation!

Rather than throwing the native exceptions, though, shouldn't we be taking the native error codes and translating them properly and/or handling them?

One way to get appropriate error strings (because the current ones are basically "there was an error" levels of useful) is with this win32 API: https://docs.microsoft.com/en-us/windows/desktop/api/iphlpapi/nf-iphlpapi-getiperrorstring

And from there we should be able to determine what should get a PingReply with an appropriate error code, and what should get a true exception (if anything should).

[scalablecory]

Thanks for the bug report. You will see this resolved in Preview 8.