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?

The `QMutex` isn't local - the `QMutexLocker`s are, and that's a different thing. — Paul Sanders, Aug 17 '20 at 23:32
@PaulSanders `wx` IS local? That's made me stumble too, I thought they lock same mutex, before I looked closer. And judging by history, the first revision in repository was this but with `mx`, then it was "fixed" because it was causing deadlocks — Swift - Friday Pie, Aug 17 '20 at 23:34
Ah right, sorry, missed that. It does look strange (and broken). — Paul Sanders, Aug 17 '20 at 23:37
I've seen the legacy code like that: the person who wrote this code doesn't know how the mutex works. — Dmitry Kuzminov, Aug 17 '20 at 23:39
@DmitryKuzminov *looks at name* I hope we aren't talking about same project — Swift - Friday Pie, Aug 17 '20 at 23:41
`mx` protects `que`, but `que.empty()` is not protected, data race for me :-/ — Jarod42, Aug 18 '20 at 00:02
I would say `QMutexLocker _wlock(&mx); while (que.empty()) { QWaitCondition::wait(&wx); } msg = que.front(); que.pop();` — Jarod42, Aug 18 '20 at 00:08

Mike Vine · Accepted Answer · 2020-08-18T00:14:05.227

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.

yeah, which results in UB because they were getting a pointer to..something.. as a `_Msg` or constantly ending with an extra item left in queue (before "fix"). This also looks like it should empty the queue in scenario they used it in because after some checks I figured out they never used same queue to "talk back" as far as I can tell — Swift - Friday Pie, Aug 18 '20 at 00:35

And odd use of conditional variable with local mutex

1 Answers1