Ping.SendAsync() always successful, even if client is not pingable in cmd

Question

I try to ping some IP addresses in C# using Ping.SendAsync() method. I have a treeView with 5 IP addresses and use the SendAsync() method for each node. Here you can see:

private void Form1_Load(object sender, EventArgs e)
{
    byte[] buffer = Encoding.ASCII.GetBytes(".");
    PingOptions options = new PingOptions(50, true);
    AutoResetEvent reset = new AutoResetEvent(false);
    Ping ping = new Ping();
    ping.PingCompleted += new PingCompletedEventHandler(ping_Complete);

    foreach (TreeNode node in treeView1.Nodes)
    {
        ping.SendAsync(node.Text, 5000, buffer, options, reset);
    }
}

private void ping_Complete(object sender, PingCompletedEventArgs k)
{
    foreach (TreeNode node in treeView1.Nodes)
    {
        PingReply reply = k.Reply;

        if (reply.Status == IPStatus.Success)
        {
            node.Text = node.Text + " (OK)";
        }

        else
        {
            node.Text = node.Text + " (FAILED)";
        }
    }
}

The problem is, the ping is always successful. I got 2 clients which were online and pingable. The other 3 are offline and not pingable (in cmd I couldn't ping these clients). So it should display this:

IP1 (OK)
IP2 (FAILED)
IP3 (FAILED)
IP4 (OK)
IP5 (FAILED)

But the output is 5 times "(OK)".

Any suggestions? :)

Answer 1

I think Jon has the correct explanation to your problem.

My suggestion is that you use the SendPingAsync method instead; it returns a Task<PingReply> which you can await (you also need to make your method asynchronous):

private async void Form1_Load(object sender, EventArgs e)
{
    byte[] buffer = Encoding.ASCII.GetBytes(".");
    PingOptions options = new PingOptions(50, true);
    AutoResetEvent reset = new AutoResetEvent(false);
    Ping ping = new Ping();
    ping.PingCompleted += new PingCompletedEventHandler(ping_Complete);

    foreach (TreeNode node in treeView1.Nodes)
    {
        var reply = await ping.SendPingAsync(node.Text, 5000, buffer, options, reset);
        if (reply.Status == IPStatus.Success)
        {
            node.Text = node.Text + " (OK)";
        }

        else
        {
            node.Text = node.Text + " (FAILED)";
        }
    }
}

(note that this approach requires .NET 4.5)

---

As pointed out by mike z in the comments, the approach above will perform the pings serially, not in parallel. If you want to do them in parallel, you can do something like that:

private async void Form1_Load(object sender, EventArgs e)
{
    byte[] buffer = Encoding.ASCII.GetBytes(".");
    PingOptions options = new PingOptions(50, true);
    AutoResetEvent reset = new AutoResetEvent(false);
    Ping ping = new Ping();
    ping.PingCompleted += new PingCompletedEventHandler(ping_Complete);

    var tasks = List<Task>();
    foreach (TreeNode node in treeView1.Nodes)
    {
        var task = PingAndUpdateNodeAsync(ping, node);
        tasks.Add(task);
    }

    await Task.WhenAll(tasks);
}

private async Task PingAndUpdateNodeAsync(Ping ping, TreeNode node)
{
    var reply = await ping.SendPingAsync(node.Text, 5000, buffer, options, reset);
    if (reply.Status == IPStatus.Success)
    {
        node.Text = node.Text + " (OK)";
    }
    else
    {
        node.Text = node.Text + " (FAILED)";
    }
}

Answer 2

Every time you get any PingCompleted event, you're updating all the nodes in your tree the same way. Instead, you should only update the node corresponding to the IP address that the particular PingCompletedEventArgs corresponds with. You might want to use the node itself as the "state" argument in the SendAsync call, so that you can use it in your event handler.

My guess is that you're either getting failures and then updating everything with successes, or you're not waiting long enough to see the failures.

Just to validate that this is diagnostically correct, I suggest you separately log reply.Status somewhere that won't be overwritten.

Additionally, you're currently updating the UI from a non-UI thread, which is a very bad idea. You should marshal back to the UI thread before updating it.