How to elegantly access thread-safe collection and use AutoResetEvent

I have two methods, ProcessQueue and AddToQueue, which happen on different threads. Sometimes I will a开发者_开发技巧ttempt to Process the Queue before an item is added to a queue, at which point I want to wait for an item to be added to a queue. I also want to make sure that I will never get a situation where I wait, after the Queue is evaluated as being empty and then after the Queue is added to on a different thread. Below is my attempt at doing this, but a deadlock is created because the Auto Reset Event waits with a lock still in force.

There has to be a more elegant way of doing this. Any suggestions?

private readonly object m_Locker = new object();
private readonly Queue<int> m_Queue = new Queue<int>();
private readonly AutoResetEvent m_AutoResetEvent = new AutoResetEvent(false);

void ProcessQueue()
{
  lock (m_Locker)
  {
    if (m_Queue.Count == 0)
    {
      // nothing is happening, so wait for it to happen
      m_AutoResetEvent.WaitOne();
    }
  }
  Console.WriteLine("Processed {0}", m_Queue.Dequeue());
}

// on another thread
void AddToQueue(int i)
{
  lock (m_Locker) 
  {
    m_Queue.Enqueue(i);
    m_AutoResetEvent.Set();
  }
}

You must release the lock on the queue m_locker before you issue the wait. You could do that manually with a Monitor, reacquire and recheck after your wait is satisfied. This way you only hold the lock while you are checking for non-zero element count.

If you are on .Net 4 you can use BlockingCollection<T> or ConcurrentQueue<T> instead, from System.Collections.Concurrent. There's really no reason to build this by hand any more.

This code won't work if you have > 1 concurrent consumer - you'd need a Semaphore instead of AutoResetEvent in that case to ensure the correct number of consumers get signaled.

Since you can't use .Net 4, there are guidelines for this scenario here. Note that the comments on that article include some approaches you can use to make this bulletproof.

The following example demonstrates thread synchronization between the primary thread and two worker threads using the lock keyword, and the AutoResetEvent and ManualResetEvent classes.

The problem is that you keep the queue locked in while you're waiting for the event.

This way the other process can't add to the queue because it is already locked. Try this:

int value = 0;

while (true)
{
    lock (m_Locker)
    {
        if (m_Queue.Count > 0)
        {
            value = m_Queue.Dequeue();
            break;
        }
    }

    m_AutoResetEvent.WaitOne();
}

With the example above, you also dequeue in the lock, so you are sure that no other thread has a chance to dequeue between the moment you waited and the moment that you check the queue actually had an item.

Well, this is textbook deadlock example. The bottom line is you don't want to enter the Wait state on your AutoResetEvent while locking on m_locker in the ProcessQueue function.

Also, note that the generic Queue implementation in .NET is not thread-safe so you should also guard access to the Dequeue call in ProcessQueue.

Wouldn't you want to do:

// no lock up here
while (true)
{
  // nothing is happening, so wait for it to happen
  m_AutoResetEvent.WaitOne();

  lock (m_locker)  
  {
  // ProcessTheQueue(); // process the queue after the reset event is Set 
  }
}

and then:

lock (m_Locker) 
  {
    m_Queue.Enqueue(i);
  }
    m_AutoResetEvent.Set();

?

If you are using .NET 4 the new BlockingCollection<T> provides the most elegant way to handle this.

Why bothering with the AutoResetEvent in the first place?

When you call the Process function, if it doesn't find anything than it should exit. I don't see the point in waiting since you'll probably just call it again after a while...