Calling QWaitCondition::wakeOne() from inside or outside my critical section?

[Bart_Vandewoestyne]

While refactoring some code written with Qt 4.8.7, I came across two pieces of code that have the same purpose, but are implemented in a slightly different way. The first version looks as follows:

void CommandHandler::endTheQueue()
{
    mutex.lock();
    queue.clear();
    Command endCommand("END");
    queue.enqueue(endCommand);
    queueNotEmpty.wakeOne();
    mutex.unlock();
}

The second version looks like this:

void CommandHandler::endTheQueue()
{
    Command endCommand("END");
    mutex.lock();
    queue.clear();
    queue.enqueue(endCommand);    
    mutex.unlock();    
    queueNotEmpty.wakeOne();
}

I see two important differences:

1. In the first example, the initialization of endCommand happens inside the critical section, while in the second example it happens outside.
2. In the first example, the queueNotEmpty.wakeOne(); call happens inside the critical section, while in the second example it happens outside of it.

I have two questions:

1. My educated guess is that the construction of endCommand should indeed happen outside the critical section. My reasoning for this is that critical sections should be as small as possible, and on top of that, endCommand is just a local variable, thus not shared data, that should not be protected.
2. I have no idea whether the call to wakeOne() should happen inside or outside the critical section. On http://doc.qt.io/qt-4.8/qwaitcondition.html#details I see example code that has a call to wakeAll() both inside and outside a critical section... Can anyone explain to me how to reason in order to decide if the call to wakeOne() should be protected by the mutex or not?

[jsulm]

@Bart_Vandewoestyne

1. Outside should be fine as it is a local variable
2. I would say it doesn't matter. If you do it inside then in the next line you do the unlock so there will be no dead-lock. But I would do it outside, because else it could happen that the next one is locked at mutex.lock(); until you do mutex.unlock();. This way you would avoid another thread being locked first before it can continue. But this should not make a big difference.

[m.sue]

Hi @Bart_Vandewoestyne

What does queueNotEmpty contain? Threads? Why are they all blocked, as wakeOne suggests? If they are blocked by the mutex of the critical section one thread will be woken automatically by mutex.unlock() and the wakeOne-call would be superfluous.

-Michael.

[Bart_Vandewoestyne]

@m.sue I'm afraid I'm not following you here, or I've not expressed myself clearly :-) The queueNotEmpty variable is of type QWaitCondition, it is not a container that contains things. The call to queueNotEmpty.wakeOne() is a call to signal that an endCommand was added to the queue and is now ready for processing/dequeueing.

[Bart_Vandewoestyne]

@jsulm I think we agree on (1). As for (2), I have been skimming through some material at https://stackoverflow.com/questions/2763714/why-do-pthreads-condition-variable-functions-require-a-mutex and most of the examples there seem to put the queueNotEmpty.wakeOne();-alike calls inside the critical section. For as far as I understand, the reasoning is that queueNotEmpty is also a shared variable (it is a private member variable of the CommandHandler class, which is in my case a Singleton that is shared among two threads) and since it can therefore be accessed by different threads, it must be protected. So following this reasoning, I would say that queueNotEmpty.wakeOne() must be inside the critical section... but of course my reasoning could be wrong :-) Any further thoughts?

[jsulm]

@Bart_Vandewoestyne From the documentation: "Note: All functions in this class are thread-safe."
http://doc.qt.io/qt-5.9/qwaitcondition.html#wakeOne
So, it should not matter.

[Bart_Vandewoestyne]

@jsulm I'm still not totally convinced, so let the discussion continue! ;-) I think the question boils down to whether

queue.clear();
queue.enqueue(endCommand); 

and

queueNotEmpty.wakeOne();

must be considered as one atomic operation or not. If 'yes', then the call to wakeOne() must be inside the critical section. If 'not', it can be left outside. I do think that in most cases it must be considered as an atomic operation, because 'updating the queue and then signalling that something is available' looks like something that must be considered 'atomic' to me... (please correct me if I'm wrong, folks!)

I've also came across this answer to a question on the Qt-interest list from 2009: http://lists.qt-project.org/pipermail/qt-interest-old/2009-June/008281.html where the author also seems to suggest to move the calls to wakeOne() inside the critical section for reasons of avoiding data races...

[m.sue]

Hi @Bart_Vandewoestyne

thread-safe means that all such operations are atomical as they are safe from other threads to interfere with their actions by definition.

-Michael.

[jsulm]

@m.sue @Bart_Vandewoestyne means that the combination of those 3 calls must be atomic. The combination is not automatically atomic just because each call is atomic.
But I don't see any reason why these 3 calls must be atomic - only access to the queue must be serialized. queueNotEmpty.wakeOne(); will be executed in any case whether it is inside the lock or outside doesn't matter in my opinion.

[Bart_Vandewoestyne]

@m.sue Consider the example from http://doc.qt.io/qt-4.8/qwaitcondition.html#details, more specifically the part

forever {
    getchar();

    mutex.lock();
    // Sleep until there are no busy worker threads
    while (count > 0) {
        mutex.unlock();
        sleep(1);
        mutex.lock();
    }
    keyPressed.wakeAll();
    mutex.unlock();
}

For as far as I understand, the topmost lock/unlock is for thread-safe access to the shared count variable, but what about the second lock/unlock? Does it only serve to protect count, or does it also protect the keyPressed condition variable? Is it intentional that keyPressed.wakeAll() is also locked, or can it be moved to after the last mutex.unlock() call? According to your reasoning, it could, but I'm not 100% convinced...

[kshegunov]

You have a thread-safe queue, as far as I can deduce from the snippet. The answer to your questions are:

1. Doesn't matter in principle, but if initialization is heavy you should move it out of the protected section. It's a good idea to keep the mutex locked for as small amount of time as possible.
2. Should be inside the mutex-locked section. This will wake the consumers to dequeue the data. In the other part of the code where dequeue is happening the waiting on the condition should be outside of the mutex-protected section.
   Edit: This is wrong.
   It's how you'd do it if you used a semaphore (which is internally realized in Qt with a mutex and a wait condition) - see the snippet here.

[Bart_Vandewoestyne]

Also consider the example code at http://doc.qt.io/qt-4.8/qt-threads-waitconditions-example.html
There, the calls to bufferNotEmpty.wakeAll() and bufferNotFull.wakeAll() are inside the critical section. My question is whether that is really necessary or not, or can those calls also be moved one statement down, to right after the mutex.unlock() statement?

I am more and more convinced that they should be in the critical section, but I can't really explain why. I currently don't have time to play with the waitconditions example code to see if things go wrong if you move the wakeAll() calls outside the critical sections, but just out of curiosity, I might experiment with it if I have some time...

[Bart_Vandewoestyne]

@kshegunov said in Calling QWaitCondition::wakeOne() from inside or outside my critical section?:

[...] 2) Should be inside the mutex-locked section. This will wake the consumers to dequeue the data. In the other part of the code where dequeue is happening the waiting on the condition should be outside of the mutex-protected section.[...]

I'm not sure if I understand your answer correctly. Our code looks something like:

mutex.lock();
if (queue.isEmpty())
{
    queueNotEmpty.wait(&mutex);
}
const auto command = queue.dequeue();
mutex.unlock();

So the wait() is inside the mutex-protected section. The documentation at http://doc.qt.io/qt-4.8/qwaitcondition.html#wait also clearly states

The mutex must be initially locked by the calling thread. If mutex is not in a locked state, this function returns immediately.

and the example at http://doc.qt.io/qt-4.8/qwaitcondition.html#details also has the wait() call inside the mutex-protected section.

Or did i interpret your answer incorrectly?

[kshegunov]

@Bart_Vandewoestyne said in Calling QWaitCondition::wakeOne() from inside or outside my critical section?:

Also consider the example code at

It's correct. That is to say:
Consumer: Wait for data to arrive before dequeueing it, then consume the data and free the producer(s).
Producer: Enqueue the data and wake up the consumers, wait for the consumers if you've produced too much.

Basically:
bufferNotFull.wait should be in the producer before adding data, respectively bufferNotFull.wakeAll - in the consumers AFTER the data has been dequeued. In reverse for the bufferNotEmpty wait condition (like in the example).

Edit:
And I didn't answer your question, sorry.

My question is whether that is really necessary or not, or can those calls also be moved one statement down, to right after the mutex.unlock() statement?

No, they're supposed to be exactly as shown in the example you sourced.

I'm not sure if I understand your answer correctly. Our code looks something like:

mutex.lock();
if (queue.isEmpty())
{
    queueNotEmpty.wait(&mutex);
}
const auto command = queue.dequeue();
mutex.unlock();

That is correct. And it is correct, because wait will unlock the mutex before waiting to be signaled. After it's been woken up it will relock it.

Edit 2: I'm still sleeping :(

Your code should be like this:

mutex.lock();
queueNotEmpty.wait(&mutex);
if (queue.isEmpty())
    ;   // Nothing more to do, exit threads.

const auto command = queue.dequeue();
mutex.unlock();