Asynchronous pinging

Question

Ran into a weird "problem". Have an application that pingsweeps entire networks. Works great until you get to a network with 255.255.0.0 netmask (which is 65k + addresses).

I send my pings like this:

    foreach (string str in ListContainingAddresses)
        {
            using (Ping ping = new Ping())
            {
                if (pingCounter == 10000) { Thread.Sleep(10000); pingCounter = 0; }
                //Make an eventhandler
                ping.PingCompleted += new PingCompletedEventHandler(pingCompleted);
                //Send the pings asynchronously
                ping.SendAsync(IPAddress.Parse(str), 1000);
                sentPings++;

                //This counts pings being sent out
                pingCounter++;
            }
        }

And recive them like this:

    public void pingCompleted(object sender, PingCompletedEventArgs e)
    {
        //This counts recieved addresses 
        recievedIpAddresses++;

        if (e.Reply.Status == IPStatus.Success)
        {
            //Do something
        }
        else
        {
            /*Computer is down*/
        }
        //This checks if sent equals recieved
        if (recievedIpAddresses == sentPings )
        {
            //All returned
        }
    }

Problem is that a) Sometimes (very rarely) it doesnt complete (Condition not met). b) When it does complete the numbers dont match? If i print sent and recieved just now they are

    Sent: 65025 Recieved: 64990

Despite this the condition is met and application moves on? I dont know why and how this is happening. Is the code executing to fast for the application to update two ints? Do some pings get lost along the way? If I try it on a subnetwork with 255 addresses this problem never happens. Cant use CountDownEvent instead of variables since its .NET 3.5

Answer 1

Do you have any locking in place at all? That looks like your problem to me. I can see all sorts of race conditions and memory processor cache issues potentially in your code.

Try using lock for protecting the recievedIpAddresses == sentPings and

sentPings++;
//This counts pings being sent out
pingCounter++;

Using lock

For instance:

private readonly object SyncRoot = new object();

public void MainMethod()
{
    foreach (string str in ListContainingAddresses)
    { ... }
    lock (SyncRoot) { sentPings++; }
    ....
}

public void pingCompleted(object sender, PingCompletedEventArgs e)
{
    //This counts recieved addresses 
    lock (SyncRoot) { recievedIpAddresses++; } // lock this if it is used on other threads

    if (e.Reply.Status == IPStatus.Success)
    {
        //Do something
    }
    else
    {
        /*Computer is down*/
    }
    lock (SyncRoot) { // lock this to ensure reading the right value of sentPings
        //This checks if sent equals recieved
        if (recievedIpAddresses == sentPings )
        {
            //All returned
        }
    }
}

The above sample will force reads and writes from shared memory so that different CPU cores aren't reading different values. However, depending on your code you may need much more coarse grained locking, where the first loop protects both sentPings and pingCounter in one lock, and maybe even the second method is completely protected with a lock.

People can say not to use lock as it causes performance issues, and lock free is very trendy. The bottom line is lock is simpler than other alternatives most of the time. You may need to make your locking more coarse grained than the above sample because you potentially have race conditions as well. It's hard to give a better sample without seeing your entire program.

Interlocked.Increment

The main reason to use lock here is to force each read and write to come from memory, not CPU cache, and therefore you should get consistent values. An alternative to lock is to use Interlocked.Increment, but if you use that on two separate variables you need to watch carefully for race conditions.

Race conditions

(Edit)

Even if you lock you potentially have a problem. Watch this timeline for 13 target addresses (unlucky for some). If you aren't comfortable with why this is, then have a look at "Managed Threading Basics" and "Threading in C# - Joseph Albahari"

• T1: 1 time
  • T1: Ping sent
  • T1: sentPings++
• T2: 1 time
  • recievedIpAddresses++;
  • T2: other stuff
• meanwhile T1: 12 times
  • T1: Ping sent
  • T1: sentPings++ (now equal to 13)
• T2: recievedIpAddresses == sentPings test - now fails as they are not equal
• T3 through to T14: enters pingCompleted and does recievedIpAddresses++;
• T1 finishes, application gets to writing out the ping counts (or worse still exits entirely) before the other 12 threads have returned in the background

You need to watch carefully for this type of race condition in your code, and adjust it accordingly. The whole thing about threads is that they overlap their operations.

SyncRoot

Footnote:

Why is SyncRoot declared as: private readonly object SyncRoot = new object();?

• It is a class field to protect class fields, if you have a static console app, it will need to be static. But, if you use static in a class, then every instance will lock on the same object, so there will be contention
• It is readonly to declare intent, and prevent you (or other team members) overwriting it later
• It is an object as:
  • you don't need anything other than an object
  • you can't lock on a value type
  • you shouldn't lock your class instance (to prevent deadlocks in more complicated code)
  • you shouldn't make it public (also to prevent deadlocks)
• It is instantiated along with the class (in a thread safe manner) by this statement
• It is called SyncRoot as an example; Visual Studio has historically called it that in it's snippets