Multi-threading problem when checking the list Count property

Question

Hi All,

I have List newJobs. Some threads add items to that list and other thread removes items from it, if it's not empty. I have ManualResetEvent newJobEvent which is set when items are added to the list, and reset when items are removed from it:

Adding items to the list is performed in the following way:

lock(syncLock){
    newJobs.Add(job);
}
newJobEvent.Set();

Jobs removal is performed in the following way:

if (newJobs.Count==0)
    newJobEvent.WaitOne();
lock(syncLock){
   job = newJobs.First();
    newJobs.Remove(job);
    /*do some processing*/
}
newJobEvent.Reset();

When the line

job=newJobs.First()

is executed I sometimes get an exception that the list is empty. I guess that the check:

 if (newJobs.Count==0)
        newJobEvent.WaitOne();

should also be in the lock statement but I'm afraid of deadlocks on the line newJobEvent.WaitOne();

How can I solve it?

Many thanks and sorry for the long post!

Answer 1

I don't see why object removal in case of zero objects should wait for one to be added and then remove it. It looks to be being against logic.

Answer 2

Not answering your question, but if you are using .NET framework 4, you can use the new ConcurrentQueue which does all the locking for you.

Regarding your question:

One scenario that I can think of causing such a problem is the following:

• The insertion thread enters the lock, calls newJob.Add, leaves the lock.
• Context switch to the removal thread. It checks for emptyness, sees an item, enters the locked area, removes the item, resets the event - which hasn't even been set yet.
• Context switch back to the insertion thread, the event is set.
• Context switch back to the removal thread. It checks for emptyness, sees no items, waits for the event - which is already set, trys to get the first item... Bang!

Set and reset the event inside the lock and you should be fine.

Answer 3

You are right. Calling WaitOne inside a lock could lead to a deadlock. And the check to see if the list is empty needs to be done inside the lock otherwise there could be a race with another thread trying to remove an item. Now, your code looks suspiciously like the producer-consumer pattern which is usually implemented with a blocking queue. If you are using .NET 4.0 then you can take advantage of the BlockingCollection class.

However, let me go over a couple of ways you can do it youself. The first uses a List and a ManualResetEvent to demonstrate how this could be done using the data structures in your question. Notice the use of a while loop in the Take method.

public class BlockingJobsCollection
{
  private List<Job> m_List = new List<Job>();
  private ManualResetEvent m_Signal = new ManualResetEvent(false);

  public void Add(Job item)
  {
    lock (m_List)
    {
      m_List.Add(item);
      m_Signal.Set();
    }
  }

  public Job Take()
  {
    while (true)
    {
      lock (m_List)
      {
        if (m_List.Count > 0)
        {
          Job item = m_List.First();
          m_List.Remove(item);
          if (m_List.Count == 0)
          {
            m_Signal.Reset();
          }
          return item;
        }
      }
      m_Signal.WaitOne();
    }
  }
} 

But this not how I would do it. I would go with the simplier solution below with uses Monitor.Wait and Monitor.Pulse. Monitor.Wait is useful because it can be called inside a lock. In fact, it is suppose to be done that way.

public class BlockingJobsCollection
{
  private Queue<Job> m_Queue = new Queue<Job>();

  public void Add(Job item)
  {
    lock (m_Queue)
    {
      m_Queue.Enqueue(item);
      Monitor.Pulse(m_Queue);
    }
  }

  public Job Take()
  {
    lock (m_Queue)
    {
      while (m_Queue.Count == 0)
      {
        Monitor.Wait(m_Queue);
      }
      return m_Queue.Dequeue();
    }
  }
}