boost locking mechanism with condition variable

Question

So I have this code, which is working good, but it seems to me that I am locking twice, and it also seems to me that I can do mistakes sometimes in case the first lock is locked again before the second one, can someone help me understand better the use of unique locks in this scenario? what is the unique lock and scoped lock doing exactly at the PrepDequeue function?

m_mtx = mutable boost::mutex

m_event = boost::condition_variable

and the enqueu and dequeue are some arbitrary queues...

Insertion function:

/** : */
void TaskImp::CmdsQueue::Enqueue(Command* cmd)
{
    boost::mutex::scoped_lock guard(m_mtx);
    bool signal = m_enqueue.empty();

    m_enqueue.push_back(cmd);
    if (signal) {
        m_event.notify_one();
    }
}

Swapping function(so dequeuing and enqueuing always works on different queues):

/** : */
void TaskImp::CmdsQueue::PrepDequeue(bool wait, size_t ms)
{   
    if (wait) {
        //QueueingMutex_t dummy; once upon a time there was a dummy here
        Timeout timeout(ms);
        boost::unique_lock<boost::mutex> lock(m_mtx); // <=== what is it waiting for?
        if (!m_event.timed_wait(lock, timeout, [this] () {
            return !this ->m_enqueue.empty();
        })) {
            return;
        }
    } 
    boost::mutex::scoped_lock guard(m_mtx); // safe lock? duplicated?
    m_enqueue.swap(m_dequeue);
}

JuanR JuanR · Accepted Answer · 2014-01-13T09:23:59

Fist I'll try to answer your question:

PrepDequeue is wating for an element to be queued, it seems that wait == true when the queue is empty.
The lock is safe since the previous lock is out of scope.

Now my thoughts:

The design is not safe, using to queues as if they were one makes no sense, synchronizing them using only one synchronization object doesn't make sense either.

The worse part is that there could be a thread other than the one running PrepDequeue, and that thread may be bloqued in a call to Dequeue, but since you are using the same synchronization object for both queues, you may ending up trying to pop() from a queue that is empty.

Is also seems that Enqueue may wakeup PrepDequeue instead.

My advice:

Use only one queue
Sycnhronize it
Get ride of PrepDequeue

You may also want to take a look to this implementation: concurrent_queue.hpp

boost locking mechanism with condition variable

1 Answers