2
\$\begingroup\$

This is a simple thread safe queue that is used in a producer-consumer model.

public class ThreadedQueue<TType>
{
    private Queue<TType> _queue;
    private object _queueLock;

    protected ThreadedQueue()
    {
        _queue = new Queue<TType>();
        _queueLock = new object();
    }

    public void Enqueue(TType data)
    {
        lock (_queueLock)
        {
            // do not allow duplicates
            if (!_queue.Contains(data))
            {
                _queue.Enqueue(data);
            }

        }
    }

    public bool TryDequeue(out TType data)
    {
        data = default(TType);
        bool success = false;
        lock (_queueLock)
        {
            if (_queue.Count > 0)
            {
                data = _queue.Dequeue();
                success = true;
            }
        }
        return success;
    }
}

Is this complete and thread safe?

\$\endgroup\$
  • 2
    \$\begingroup\$ What's wrong with ConcurrentQueue<T>? \$\endgroup\$ – svick May 25 '12 at 7:53
  • \$\begingroup\$ I am not sure it is thread safe. \$\endgroup\$ – Odys May 25 '12 at 8:14
  • 2
    \$\begingroup\$ The point of ConcurrentQueue<T> is that it is thread-safe. It says so at the very top of the documentation: “Represents a thread-safe first in-first out (FIFO) collection.” \$\endgroup\$ – svick May 25 '12 at 11:35
  • \$\begingroup\$ I am on .Net 3.5 sorry for not mentioning it earlier. ConcurrentQueue<T> isn't available \$\endgroup\$ – Odys May 25 '12 at 12:00
2
\$\begingroup\$

This looks mostly fine by me (ignoring Reflection and other more obscure sources of thread safety failures), but I wouldn't call it a Queue per say (because of the duplicate check there). Does it really matter what order the set of items in this collection are in (if so then the duplicate check introduces a bug in that ordering, if not then why not call this a Set)?

One change you should really make is adding readonly to your private members.

Also, while this looks thread safe, there is no reason to assume that it is safe to use any of the items inside of it or any inherited versions of it. Consider code like this (intended to obviously be unsafe):

public class NotThreaSafeExampleClass {
    public Action<string> SayHello;
}

//thread 1: (q = shared ThreadedQueue<NotThreaSafeExampleClass>)
var mean = new NotThreaSafeExampleClass();
q.Enqueue(mean);
while (true) {
    mean.SayHello = name => { Console.WriteLine("Hi "+name); };
    Thread.Sleep(10);
    mean.SayHello = null;
    Thread.Sleep(10);
}

//thread 2:
NotThreaSafeExampleClass otherThreadMethod;
q.TryDequeue(out otherThreadMethod);
while (true) { otherThreadMethod.SayHello('odyodyodys'); Thread.Sleep(15);}

Lock based concurrency in .NET is a fairly simple paradigm (compared to the other ways of ensuring thread safety in .NET). When you lock, you declare a block of code as a critical section, protected by that lock object. .NET ensures that a second piece of code in the same app domain will never be running in a critical section protected by the same lock object while one instance is. Here is a good resource for more threading information: http://www.albahari.com/threading/part2.aspx

Pedantry

When most people discuss thread safety they mean it in the same sense as saying strings are immutable. That is to say, as long as you don't do anything I shouldn't reasonably expect you might do it is safe.

Unfortunately, locks aren't good enough to actually provide true thread safety (bulletproof against reflection and unsafe C# pointer code), they are good enough for most usages where you control the software stack out to the actual application. If you need to ensure thread safety in these instances you must ensure thread safety at the field level in your classes (stuff like the volatile keyword will help you here, but watch out).

Some example threads that would break your safety here:

//as an additional thread running in the app, this would effectively make the locks no longer exclusive
var l = q.GetType().GetField("_queueLock", BindingFlags.NonPublic|BindingFlags.Instance);
while (true) { l.SetValue(q,new object()); }
//making the lock object readonly would protect against this

//ex 2:
var qf = q.GetType().GetField("_queue", BindingFlags.NonPublic|BindingFlags.Instance);
var field = (Queue<TType>)qf.GetValue(q);
field.Clear(); //what if this will happen between the Count check and the Dequeue statement
//readonly will not protect against this

While these examples are pretty far out there, consider if your Queue class were complex enough that I might want to add an extension method to it and need access to internal variables in the method to do some algorithm in a way much better than possible with the available public interface.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.