Unsolved Calling QWaitCondition::wakeOne() from inside or outside my critical section?
-
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:
- In the first example, the initialization of
endCommand
happens inside the critical section, while in the second example it happens outside. - 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:
- 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. - 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 towakeAll()
both inside and outside a critical section... Can anyone explain to me how to reason in order to decide if the call towakeOne()
should be protected by the mutex or not?
- In the first example, the initialization of
-
- Outside should be fine as it is a local variable
- 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.
-
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.
-
@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 toqueueNotEmpty.wakeOne()
is a call to signal that an endCommand was added to thequeue
and is now ready for processing/dequeueing. -
@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 thatqueueNotEmpty
is also a shared variable (it is a private member variable of theCommandHandler
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 thatqueueNotEmpty.wakeOne()
must be inside the critical section... but of course my reasoning could be wrong :-) Any further thoughts? -
@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. -
@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... -
thread-safe means that all such operations are atomical as they are safe from other threads to interfere with their actions by definition.
-Michael.
-
@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. -
@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 protectcount
, or does it also protect thekeyPressed
condition variable? Is it intentional thatkeyPressed.wakeAll()
is also locked, or can it be moved to after the lastmutex.unlock()
call? According to your reasoning, it could, but I'm not 100% convinced... -
You have a thread-safe queue, as far as I can deduce from the snippet. The answer to your questions are:
- 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.
- Should be inside the mutex-locked section. This will wake the consumers to
dequeue
the data. In the other part of the code wheredequeue
is happening the waiting on the condition should beoutside 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.
-
Also consider the example code at http://doc.qt.io/qt-4.8/qt-threads-waitconditions-example.html
There, the calls tobufferNotEmpty.wakeAll()
andbufferNotFull.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 themutex.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... -
@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 wheredequeue
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 statesThe 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?
-
@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, respectivelybufferNotFull.wakeAll
- in the consumers AFTER the data has been dequeued. In reverse for thebufferNotEmpty
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, becausewait
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();