patterncppMajor
A multi-threaded Producer Consumer with C++11
Viewed 0 times
multiwithconsumerproducerthreaded
Problem
I am trying to learn concurrent programming in C++11. I tried to write code for a classic producer consumer concurrency problem. Would you please review and make any comments about it?
#include
#include
#include
#include
#include
#include
using std::deque;
std::mutex mu,cout_mu;
std::condition_variable cond;
class Buffer
{
public:
void add(int num) {
while (true) {
std::unique_lock locker(mu);
cond.wait(locker, [this](){return buffer_.size() locker(mu);
cond.wait(locker, [this](){return buffer_.size() > 0;});
int back = buffer_.back();
buffer_.pop_back();
locker.unlock();
cond.notify_all();
return back;
}
}
Buffer() {}
private:
deque buffer_;
const unsigned int size_ = 10;
};
class Producer
{
public:
Producer(Buffer* buffer)
{
this->buffer_ = buffer;
}
void run() {
while (true) {
int num = std::rand() % 100;
buffer_->add(num);
cout_mu.lock();
std::cout buffer_ = buffer;
}
void run() {
while (true) {
int num = buffer_->remove();
cout_mu.lock();
std::cout << "Consumed: " << num << std::endl;
std::this_thread::sleep_for(std::chrono::milliseconds(50));
cout_mu.unlock();
}
}
private:
Buffer *buffer_;
};
int main() {
Buffer b;
Producer p(&b);
Consumer c(&b);
std::thread producer_thread(&Producer::run, &p);
std::thread consumer_thread(&Consumer::run, &c);
producer_thread.join();
consumer_thread.join();
getchar();
return 0;
}Solution
Prefer Reference over Pointer
Since the producer and consumer must have a buffer you should pass it by reference (rather than pointer). This also makes sure there is no confusion over ownership of the buffer (the owner of a pointer is responsible for deleting it). By using a RAW pointer you can not tell the owner but by using a reference you are explicitly stating that you are not passing ownership.
Prefer to avoid this->
When you use
Member variables over globals.
Looks like your encapsulation of the
Because they are global, you use a single mutex/condition for all buffer objects. This looks like a design flaw. It seems like the mutex/condition belongs to the class so that you are just locking the buffer you are manipulating (so you can use multiple buffers in the same application).
Sleeping inside lock
@Morwenn already suggested using scoped lock to make sure the mutex were correctly locked and unlocked. But I would also move the sleep outside the lock. This causes the current thread to stall but the other thread can not continue as the lock is still held while it is sleeping.
Prefer '\n' over
The difference between the two is a flush. Usually you do not want to manually flush (the stream have good flushing techniques built in). So it is usually best to let the streams flush themselves when appropriate.
In answer to comments:
Since the producer and consumer must have a buffer you should pass it by reference (rather than pointer). This also makes sure there is no confusion over ownership of the buffer (the owner of a pointer is responsible for deleting it). By using a RAW pointer you can not tell the owner but by using a reference you are explicitly stating that you are not passing ownership.
class Producer
{
Buffer& buffer_;
public:
Producer(Buffer& buffer)
: buffer_(buffer)
{}Prefer to avoid this->
When you use
this-> it means you have scoping issues with your variables (which is a code smell for bad design). Use accurate variable names so that there is no confusion on where the variables belongs.Member variables over globals.
Looks like your encapsulation of the
mu and cond is wrong.std::mutex mu,cout_mu;Because they are global, you use a single mutex/condition for all buffer objects. This looks like a design flaw. It seems like the mutex/condition belongs to the class so that you are just locking the buffer you are manipulating (so you can use multiple buffers in the same application).
Sleeping inside lock
cout_mu.lock();
std::cout << "Produced: " << num << std::endl;
std::this_thread::sleep_for(std::chrono::milliseconds(50));
cout_mu.unlock();@Morwenn already suggested using scoped lock to make sure the mutex were correctly locked and unlocked. But I would also move the sleep outside the lock. This causes the current thread to stall but the other thread can not continue as the lock is still held while it is sleeping.
{
std::unique_lock locker(cout_mu);
std::cout << "Produced: " << num << std::endl;
}
std::this_thread::sleep_for(std::chrono::milliseconds(50));Prefer '\n' over
std::endlstd::cout << "Produced: " << num << std::endl;The difference between the two is a flush. Usually you do not want to manually flush (the stream have good flushing techniques built in). So it is usually best to let the streams flush themselves when appropriate.
In answer to comments:
class Buffer
{
public:
void add(int num) {
while (true) {
std::unique_lock locker(mu);
cond.wait(locker, [this](){return buffer_.size() locker(mu);
cond.wait(locker, [this](){return buffer_.size() > 0;});
int back = buffer_.back();
buffer_.pop_back();
locker.unlock();
cond.notify_all();
return back;
}
}
Buffer() {}
private:
// Add them as member variables here
std::mutex mu;
std::condition_variable cond;
// Your normal variables here
deque buffer_;
const unsigned int size_ = 10;
};Code Snippets
class Producer
{
Buffer& buffer_;
public:
Producer(Buffer& buffer)
: buffer_(buffer)
{}std::mutex mu,cout_mu;cout_mu.lock();
std::cout << "Produced: " << num << std::endl;
std::this_thread::sleep_for(std::chrono::milliseconds(50));
cout_mu.unlock();{
std::unique_lock<std::mutex> locker(cout_mu);
std::cout << "Produced: " << num << std::endl;
}
std::this_thread::sleep_for(std::chrono::milliseconds(50));std::cout << "Produced: " << num << std::endl;Context
StackExchange Code Review Q#84109, answer score: 31
Revisions (0)
No revisions yet.