Since I learned C++ reading one of the first edition of TC++PL and even if I'm programming using C++ since 2000 or so I'm reading the new TC++PL4 carefully as if this language is totally new to me. It seems my last post about an error found on this book will be not the unique and here we are again.
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 mqueue;
condition_variable mcond;
mutex mmutex;
void consumer()
{
while(true) {
unique_lock lck{mmutex};
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 lck{mmutex};
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 lck{mmutex};
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 lck{mmutex};
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
to address it the producer has to "forecast" if the consumer can be in a blocked status, and this can happen only if after having acquired the mmutex the queue is empty, this is the final version of producer:
void producer()
{
while(true) {
Message m;
bool notifyIsNeeded = false;
// ... fill the message ...
{
unique_lock lck{mmutex};
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.
