Asynchronous ping

Question

Falls into a strange "problem". Have an application that supports the entire network. It works fine until you get a network with a network network of 255.255.0.0 (which is 65 thousand + Addresses).

I send my messages as follows:

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 get 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 } } 

The problem is that a) Sometimes (very rarely) it does not end (the condition is not fulfilled). b) When does it end, the numbers do not match? If I print sent and received just now, they

  Sent: 65025 Recieved: 64990 

Despite this, the condition is met and the application moves on? I do not know why and how this happens. Is code running fast for an application to update two ints? Are some pings lost along the way? If I try it on a subnet with 255 addresses, this problem will never happen. Do not use the CountDownEvent parameter instead of variables, as its .NET 3.5

Answer 1

Do you have any kind of lock at all? This seems like your problem to me. I see in your code all kinds of race conditions and problems with the memory processor cache .

Try using lock to protect 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 example will force you to read and write data from shared memory so that different CPU cores do not read different values. However, depending on your code, you may need a much coarse-grained lock, where the first loop protects both sentPings and pingCounter in one lock , and perhaps even the second method is fully protected by lock .

People may say that do not use lock , as it causes performance problems, and locking is very fashionable. The bottom line of lock in most cases simpler than other alternatives. You may need to make your lock more coarse than the above sample, because you also have race conditions. It’s hard to give a better sample without seeing your entire program.

Interlocked.Increment

The main reason for using lock here is to force every read and write to come from memory, not from the CPU cache, and so you should get consistent values. An alternative to locking is to use Interlocked.Increment , but if you use this on two separate variables, you need to carefully monitor the race conditions.

Race conditions

(Edit)

Even if you block, you may have a problem. Check out this timeline for 13 destination addresses (unfortunately for some). If you are uncomfortable with why this is the case, take 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 things
• Meanwhile T1: 12 times
  • T1: Ping sent
  • T1: sentPings++ (now equal to 13)
• T2: recievedIpAddresses == sentPings test - now fails because they are not equal
• T3 to T14: pingCompleted enters and does recievedIpAddresses++;
• T1 ends, the application gets the opportunity to record ping counts (or, even worse, completely exits) before the remaining 12 threads return in the background

You need to carefully monitor this type of race condition in your code and adjust it accordingly. The thing about flows is that they overlap their operations.

Syncroot

Footnote:

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

• This is a class field to protect class fields, if you have a static console application, it should be static . But, if you use static in the class, then each instance will block the same object, so there will be conflicts
• readonly declare intent and stop you (or other team members) from rewriting it later
• This is an object like:
  • you need nothing but an object
  • you cannot block value type
  • you should not lock your class instance (to prevent deadlocks in more complex code)
  • you should not publish it (also to prevent deadlocks)
• It is created together with the class (safe thread) using this operator
• It is called SyncRoot as an example; Visual Studio historically called it fragments in it