In 5.3.4.1 he introduces conditions and how two threads can interact each other communicating using events, it's presented the classical producer / consumer interaction exchanging Messages trough a queue, and this is the poor implementation proposed:
class Message {
// ...
};
queue
condition_variable mcond;
mutex mmutex;
void consumer()
{
while(true) {
unique_lock
while (mcond.wait(lck)) /* do nothing */;
auto m = mqueue.front();
mqueue.pop();
lck.unlock();
// ... process m...
}
}
void producer()
{
while(true) {
Message m;
// ... fill the message ...
unique_lock
mqueue.push(m);
mcond.notify_one();
}
}
This implementation is affected by at least three issues:
- Unless very lucky the queue will grow indefinitely: that's because basically the consumer will wait at each cycle even if queue contains something, at the same time it has a chance (I repeat a "chance") to exit from the condition_variable::wait() only each time the producer puts something in the queue.
- The consumer can miss the condition_variable::notify_one event, indeed if the producer does the notify_one() but the other thread hasn't yet executed the wait() the consumer will block for no reason
- The producer holds the unique_lock for more time than needed, the mutex has to only protect the queue not the condition as well
Let see how those producer / consumer should have been implemented:
void consumer()
{
while(true) {
unique_lock
while (mqueue.empty()) { // the empty condition has to be recheck indeed the thread
// can get sporadic wakeup without any thread doing a notify
mcond.wait(lck);
}
auto m = mqueue.front();
mqueue.pop();
lck.unlock();
// ... process m...
}
}
void producer()
{
while(true) {
Message m;
// ... fill the message ...
{
unique_lock
mqueue.push(m);
} // This extra scope is in here to release the mmutex asap
mcond.notify_one();
}
}
In my opinion this should have been the version of the producer/consumer in TC++PL4, as you can see
with a simple extra scope and the right while(...) the issues reported in the bullets are solved.
There is another problem, I have to admit that this issue most of the times is a minore one:
- The producer can issue notify_one() even if not needed, and this can be a performance issue
void producer()
{
while(true) {
Message m;
bool notifyIsNeeded = false;
// ... fill the message ...
{
unique_lock
if (mqueue.empty()) {
notifyIsNeeded = true;
}
mqueue.push(m);
} // This extra scope is in here to release the mmutex asap
if ( notifyIsNeeded ) {
mcond.notify_one();
}
}
}
Writing correct code is not easy and writing correct multi-threaded is damn hard.
No comments:
Post a Comment