I have started learning multi-threading using C++ 11 and here I have posted the code for thread-safe FIFO queue for multiple producers and consumers.
Though it works fine but I want to know any suggestions,best practices or even any loop hole if you guys can find it.
#ifndef BOUNDEDBUFFER_H
#define BOUNDEDBUFFER_H
#include <thread>
#include <mutex>
#include <condition_variable>
#include <iostream>
// T must implement operator <<
template< typename T>
class BoundedBuffer
{
public:
BoundedBuffer(size_t size) :m_size(size), m_front(0), m_rear(-1), m_count(0)
{
m_queue = new T[m_size];
}
void enqueue( const T& obj )
{
std::unique_lock<std::mutex> writerLock(m_mutex);
while ( m_size == m_count )
{
writers.wait(writerLock);
}
m_rear = (m_rear + 1) % m_size;
m_queue[m_rear] = obj;
++m_count;
std::cout << std::this_thread::get_id() << " has Enqueued Object =" << obj << std::endl;
readers.notify_all();
}
void dequeue( T& obj )
{
std::unique_lock<std::mutex> readerLock(m_mutex);
while (0 == m_count)
{
readers.wait(readerLock);
}
obj = m_queue[m_front];
m_front = (m_front + 1) % m_size;
--m_count;
std::cout << std::this_thread::get_id() << " has Dequeued Object =" << obj << std::endl;
writers.notify_all();
}
~BoundedBuffer()
{
if (m_queue)
{
delete[] m_queue;
}
}
private:
size_t m_size;
T * m_queue;
int m_front, m_rear, m_count;
std::mutex m_mutex;
std::condition_variable readers;
std::condition_variable writers;
};
#endif
1 Answer 1
If the allocation in the ctor throws, the dtor will have to contend with an uninitialized owning raw pointer. In other words, you have Undefined Behavior.
The minimal fix would be value-initializing
m_queue
before trying to allocate the memory. Moving the allocation into the ctor-init-list would not work, as constructing astd::condition_variable
can throw.The better fix would be using a
std::unique_ptr<T[]>
, and maybe allocating the memory with a back-port of C++14std::make_unique
, though assigning in the ctor-body would then also work.delete [] pointer;
andif(pointer) delete [] pointer
differ in exactly one point:
The second is longer.Not that you should have any raw owning pointers.
You know remainder (
%
) is a fairly expensive operation? And the dividend is in the range [0, divisor] inclusive.You can suppress the notification of writers if the buffer wasn't full before the call to
dequeue()
.Likewise, you can suppress the notification of readers if the buffer wasn't empty before
dequeue()
.You should direct debug-output to
std::cerr
instead. Which would obviate the need for manually flushing withstd::endl
.Of course, the debug-output should be removed from the release version.
Strictly speaking, one of
m_front
,m_rear
is superfluous because you also havem_count
.It's a curious decision to make
m_rear
always point to the last object youenqueue()
d, instead of past it, and thus adjusting it before you actually copy the object. It means thatenqueue()
depends on copy-assignment ofT
not throwing for correctness.Your decision to use different names for the
std::unique_lock
inenqueue()
anddequeue()
threw me for a loop. For a moment I thought there was a reason for them, namely two different mutexes. Better change that...