patterncppMinor
Implementation of concurrent blocking queue for producer-consumer
Viewed 0 times
blockingconsumerforimplementationproducerconcurrentqueue
Problem
I've implemented
I intend it to block the producers as well, when the queue is full, which is why I set the capacity of the queue in the constructor. I've provided my own implementation of
Please review it. I want to know if there is any potential deadlocks.
And the remaining classes are posted below. Note that I've implemented them in
concurrent_blocking_queue for multiple-producers and single consumer.I intend it to block the producers as well, when the queue is full, which is why I set the capacity of the queue in the constructor. I've provided my own implementation of
critical_section, scoped_lock and condition_variable, using msdn primitives.Please review it. I want to know if there is any potential deadlocks.
template
class concurrent_blocking_queue : public noncopyable
{
std::queue m_internal_queue;
mutable critical_section m_critical_section;
condition_variable m_queue_full;
condition_variable m_queue_empty;
const size_t m_capacity;
public:
concurrent_blocking_queue(size_t capacity)
: m_capacity(capacity), m_critical_section()
{
}
void put(T const & input)
{
framework::scoped_lock lock(m_critical_section); //lock
m_queue_full.wait(lock, [&]{ return !full(); });
m_internal_queue.push(input);
m_queue_empty.notify_all();
}
T take()
{
framework::scoped_lock lock(m_critical_section); //lock
m_queue_empty.wait(lock, [&]{ return !empty(); });
T output = m_internal_queue.front();
m_internal_queue.pop();
m_queue_full.notify_all();
return output;
}
bool full() const
{
framework::scoped_lock lock(m_critical_section);
if ( m_internal_queue.size() > m_capacity )
{
throw std::logic_error("size of concurrent_blocking_queue cannot be greater than the capacity.");
}
return m_internal_queue.size() == m_capacity;
}
bool empty() const
{
framework::scoped_lock lock(m_critical_section);
return m_internal_queue.empty();
}
//..
};And the remaining classes are posted below. Note that I've implemented them in
.h and .cpp files, but here I've merged them to make the post short and simple, and have removed the namespace for brevity, othSolution
Basic comments:
No point in functions that do nothing.
Also avoid
Why put a member that has no default constructor in the initializer list ( I can go either way)
But you should be consistent and thus if you do it for one then you should do it for all members.
Design:
When you call
Thus I would change the above to call a private version of these function that do not use locks.
No point in functions that do nothing.
Also avoid
void as definition for empty parameter list.~condition_variable(void) {}Why put a member that has no default constructor in the initializer list ( I can go either way)
concurrent_blocking_queue(size_t capacity)
: m_capacity(capacity), m_critical_section()But you should be consistent and thus if you do it for one then you should do it for all members.
Design:
When you call
full() and empty() from within other members you already have a lock. Yet you are calling a function that will add another level of locking. And locking is usually relatively expensive.m_queue_full.wait(lock, [&]{ return !full(); });
m_queue_empty.wait(lock, [&]{ return !empty(); });Thus I would change the above to call a private version of these function that do not use locks.
// OLD STUFF
m_queue_full.wait(lock, [&]{ return !test_full(); });
// OLD STUFF
m_queue_empty.wait(lock, [&]{ return !test_empty(); });
bool full() const
{
framework::scoped_lock lock(m_critical_section);
return test_full();
}
bool empty() const
{
framework::scoped_lock lock(m_critical_section);
return test_empty();
}
private:
bool test_full() const {STUFF}
bool test_empty() const {STUFF}Code Snippets
~condition_variable(void) {}concurrent_blocking_queue(size_t capacity)
: m_capacity(capacity), m_critical_section()m_queue_full.wait(lock, [&]{ return !full(); });
m_queue_empty.wait(lock, [&]{ return !empty(); });// OLD STUFF
m_queue_full.wait(lock, [&]{ return !test_full(); });
// OLD STUFF
m_queue_empty.wait(lock, [&]{ return !test_empty(); });
bool full() const
{
framework::scoped_lock lock(m_critical_section);
return test_full();
}
bool empty() const
{
framework::scoped_lock lock(m_critical_section);
return test_empty();
}
private:
bool test_full() const {STUFF}
bool test_empty() const {STUFF}Context
StackExchange Code Review Q#12052, answer score: 3
Revisions (0)
No revisions yet.