views:

157

answers:

3

I've been using this code as a queue that blocks on Dequeue() until an element is enqueued. I've used this code for a few years now in several projects, all with no issues... until now. I'm seeing a deadlock in some code I'm writing now, and in investigating the problem, my 'eye of suspicion' has settled on this BlockingQueue<T>. I can't prove it, so I figured I'd ask some people smarter than me to review it for potential issues. Can you guys see anything that might cause a deadlock in this code?

public class BlockingQueue<T>
{
    private readonly Queue<T> _queue;
    private readonly ManualResetEvent _event;

    /// <summary>
    /// Constructor
    /// </summary>
    public BlockingQueue()
    {
        _queue = new Queue<T>();
        _event = new ManualResetEvent(false);
    }

    /// <summary>
    /// Read-only property to get the size of the queue
    /// </summary>
    public int Size
    {
        get
        {
            int count;

            lock (_queue)
            {
                count = _queue.Count;
            }

            return count;
        }
    }

    /// <summary>
    /// Enqueues element on the queue
    /// </summary>
    /// <param name="element">Element to enqueue</param>
    public void Enqueue(T element)
    {
        lock (_queue)
        {
            _queue.Enqueue(element);
            _event.Set();
        }
    }

    /// <summary>
    /// Dequeues an element from the queue
    /// </summary>
    /// <returns>Dequeued element</returns>
    public T Dequeue()
    {
        T element;

        while (true)
        {
            if (Size == 0)
            {
                _event.Reset();
                _event.WaitOne();
            }

            lock (_queue)
            {
                if (_queue.Count == 0) continue;

                element = _queue.Dequeue();
                break;
            }
        }

        return element;
    }

    /// <summary>
    /// Clears the queue
    /// </summary>
    public void Clear()
    {
        lock (_queue)
        {
            _queue.Clear();
        }
    }
}
+1  A: 

This code is broken in several ways. Here's one scenario. There's a race-condition between if (Size == 0) and _event.Reset(). An Enqueue might fire between the two, and its signal will be lost.

An unounded-length BlockingQueue is much more easily implemented with a semaphore.

Marcelo Cantos
I appreciate the response, and I'll look into those areas, but please, the attitude isn't needed. I'm asking for help. Your answer has helped me reevaluate this, and I've found another BlockingQueue on SO that I'm going to look at using (http://stackoverflow.com/questions/530211/creating-a-blocking-queuet-in-net/530228#530228)
unforgiven3
@Marcelo: I don't think your first paragraph is true. Since he's only calling `_event.WaitOne` if the size is 0, there will never be a state when "it's no longer possible to empty the queue out"; rather, `Dequeue` will block a lot longer than it needs to. But a fresh call to `Enqueue` will always be all it takes to get the queue back in shape (until the next race condition). Or am I missing something?
Dan Tao
My apologies to anyone I offended; particularly the OP. I was under the impression that the OP was using someone else's library and was wondering whether to retain it or not. I have a pretty hefty track record that you can observe for yourselves, in case you think that this is response is typical of me. FWIW, I've amended the answer.
Marcelo Cantos
+5  A: 

I think this could be your problem:

Thread 1                    Thread 2
Dequeue
                            Enqueue    
if (Size == 0)                              // Thread 1 gets the lock
                            lock (_queue)   // Thread 2 has to wait
return _queue.Count                         // Thread 1 sees: Size == 0
                            _queue.Enqueue  // Thread 2 gets the lock
                            _event.Set    
_event.Reset                                // uh oh
_event.WaitOne                              // now Dequeue's going to block
                                            // until Enqueue gets called again
                                            // (even though queue isn't empty)
Dan Tao
This is a great way to visualize it. Thank you, I actually think this is the exact situation I'm running into. It's amazing that I've never seen it before now (I've been using this for a few years).
unforgiven3
@unforgiven3: Well, maybe not *so* amazing when you consider how precisely this scenario has to occur for the race condition to bite you. This is why writing multithreaded code can give developers nightmares. (I know it does to *me*, anyway!)
Dan Tao
@Dan Tao, yeah, my worst code nightmares are about multithreaded code as well, haha.
unforgiven3
A: 

I don't know about your requirements or what else your class does, but if you can use .NET 4, you might want to consider using ConcurrentQueue<T> and BlockingCollection<T> which, used together, should give you a blocking queue.

Chris Schmich