And odd use of conditional variable with local mutex

Question

Poring through legacy code of old and large project, I had found that there was used some odd method of creating thread-safe queue, something like this:

template < typename _Msg>
class WaitQue: public QWaitCondition 
{
public:
    typedef _Msg  DataType;

    void wakeOne(const DataType& msg) 
    {
        QMutexLocker lock_(&mx);
        que.push(msg);
        QWaitCondition::wakeOne(); 
    }
    
    void wait(DataType& msg)
    {
        /// wait if empty.
        {
            QMutex wx;  // WHAT?
            QMutexLocker cvlock_(&wx);
            if (que.empty()) 
                QWaitCondition::wait(&wx);  
        }

        {
            QMutexLocker _wlock(&mx);
            msg = que.front();            
            que.pop();
        }
    }

    unsigned long size() { 
        QMutexLocker lock_(&mx); 
        return que.size();
    }

private:
    std::queue<DataType> que;

    QMutex mx;
};

wakeOne is used from threads as kind of "posting" function" and wait is called from other threads and waits indefinitely until a message appears in queue. In some cases roles between threads reverse at different stages and using separate queues.

Is this even legal way to use a QMutex by creating local one? I kind of understand why someone could do that to dodge deadlock while reading size of que but how it even works? Is there a simpler and more idiomatic way to achieve this behavior?

Answer 1

Its legal to have a local condition variable. But it normally makes no sense.

As you've worked out in this case is wrong. You should be using the member:

void wait(DataType& msg)
{
    QMutexLocker cvlock_(&mx);
    while (que.empty()) 
        QWaitCondition::wait(&mx);

    msg = que.front();            
    que.pop();
}

Notice also that you must have while instead of if around the call to QWaitCondition::wait. This is for complex reasons about (possible) spurious wake up - the Qt docs aren't clear here. But more importantly the fact that the wake and the subsequent reacquire of the mutex is not an atomic operation means you must recheck the variable queue for emptiness. It could be this last case where you previously were getting deadlocks/UB.

Consider the scenario of an empty queue and a caller (thread 1) to wait into QWaitCondition::wait. This thread blocks. Then thread 2 comes along and adds an item to the queue and calls wakeOne. Thread 1 gets woken up and tries to reacquire the mutex. However, thread 3 comes along in your implementation of wait, takes the mutex before thread 1, sees the queue isn't empty, processes the single item and moves on, releasing the mutex. Then thread 1 which has been woken up finally acquires the mutex, returns from QWaitCondition::wait and tries to process... an empty queue. Yikes.