Is this the right approach for a thread-safe Queue class?

Question

Hey guys,

I'm wondering if this is the right approach to writing a thread-safe queue in C++?

template <class T>
class Queue
{
public:

Queue() {}

void Push(T& a)
{
    m_mutex.lock();
    m_q.push_back(a);
    m_mutex.unlock();
}

T& Pop()
{
    m_mutex.lock();
    T& temp = m_q.pop();
    m_mutex.unlock();
    return temp;
}

private:
    std::queue<t> m_q;
    boost::mutex m_mutex;
};

You get the idea... I'm just wondering if this is the best approach. Thanks!

EDIT: Because of the questions I'm getting, I wanted to clarify that the mutex is a boost::mutex

Answer 1

From a threading-point of view, that looks about right for a simple, thread-safe queue.

You do have one problem, though: std::queue's pop() does not return the element popped from the queue. What you need to do is:

T Pop()
{
    m_mutex.lock();
    T temp = m_q.front();
    m_q.pop();
    m_mutex.unlock();
    return temp;
}

You don't want to return a reference in this case since the referenced element is being popped from the queue and destroyed.

You also need to have some public Size() function to tell you how many elements are in the queue (either that, or you'll need to gracefully handle the case where Pop() is called and there are no elements in the queue).

Edit: Though, as Eli Courtwright points out, you do have to be careful with the queue operations throwing exceptions, and using Boost is a good idea.

Answer 2

Depends what are your goals. Crafted in this manner, you'll have your "reader" client blocking your "writer" client. You might want to consider using a "condition" to avoid dead-locks etc.

Answer 3

I recommend using the Boost threading libraries to assist you with this.

Your code is fine, except that when you write code in C++ like

some_mutex.lock();
// do something
some_mutex.unlock();

then if the code in the // do something section throws an exception then the lock will never be released. The Boost library solves this with its classes such as lock_guard in which you initialize an object which acquires a lock in its constructor, and whose destructor releases the lock. That way you know that your lock will always be released. Other languages accomplish this through try/finally statements, but C++ doesn't support this construct.

In particular, what happens when you try to read from a queue with no elements? Does that throw an exception? If so, then your code would run into problems.

When trying to get the first element, you probably want to check if something is there, then go to sleep if not and wait until something is. This is a job for a condition object, also provided by the Boost library, though available at a lower level if you prefer.

Answer 4

The approach that your are trying to implement is a locking approach. It will work, except that if you use a plain system-provided "mutex" object, it's performance might turn out disappointing (its lock-unlock overhead is pretty high). It is hard to say whether it will be good or not, since we don't know what your performance requirements and expectations are.

Since the operations you perform in "locked" segments of your code are rather quick, it might make sense to use a spin-lock instead of a true mutex, or a combination of the two. This will give you much better performance. Then again, maybe your "mutex" already implements all that (no way to know, since you provided no details about what is actually hiding behind that name).

And finally, if you are happen to be looking for best performance, you might want to read up on lock-free synchronization approach, which is a completely different story. Lock-free methods are typically much more difficult to implement though.

Answer 5

Herb Sutter wrote an excellent article last year in Dr. Dobbs Journal, covering all of the major concerns for a thread-safe, lock-free, single-producer, single-consumer queue implementation. (Which made corrections over an implementation published the previous month.)

His followup article in the next issue tackled a more generic approach for a multi-user concurrent queue, along with a full discussion of potential pitfalls and performance issues.

There are a few more articles on similar concurrency topics.

Enjoy.