c# - How to do proper Parallel.ForEach, locking and progress reporting

Question

I'm trying to implement the Parallel.ForEach pattern and track progress, but I'm missing something regarding locking. The following example counts to 1000 when the threadCount = 1, but not when the threadCount > 1. What is the correct way to do this?

class Program
{
   static void Main()
   {
      var progress = new Progress();
      var ids = Enumerable.Range(1, 10000);
      var threadCount = 2;

      Parallel.ForEach(ids, new ParallelOptions { MaxDegreeOfParallelism = threadCount }, id => { progress.CurrentCount++; });

      Console.WriteLine("Threads: {0}, Count: {1}", threadCount, progress.CurrentCount);
      Console.ReadKey();
   }
}

internal class Progress
{
   private Object _lock = new Object();
   private int _currentCount;
   public int CurrentCount
   {
      get
      {
         lock (_lock)
         {
            return _currentCount;
         }
      }
      set
      {
         lock (_lock)
         {
            _currentCount = value;
         }
      }
   }
}

Answer 1

The usual problem with calling something like count++ from multiple threads (which share the count variable) is that this sequence of events can happen:

1. Thread A reads the value of count.
2. Thread B reads the value of count.
3. Thread A increments its local copy.
4. Thread B increments its local copy.
5. Thread A writes the incremented value back to count.
6. Thread B writes the incremented value back to count.

This way, the value written by thread A is overwritten by thread B, so the value is actually incremented only once.

Your code adds locks around operations 1, 2 (get) and 5, 6 (set), but that does nothing to prevent the problematic sequence of events.

What you need to do is to lock the whole operation, so that while thread A is incrementing the value, thread B can't access it at all:

lock (progressLock)
{
    progress.CurrentCount++;
}

If you know that you will only need incrementing, you could create a method on Progress that encapsulates this.