Unsolved Calling QWaitCondition::wakeOne() from inside or outside my critical section?
-
@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();