boost::thread producer consumer

Question

I am new toboost::thread and am making a producer-consumer with a Monitor. This is how I've coded it so far:

//{ Declarations in header
private:
  boost::condition_variable    _condition;
  boost::mutex                 _mutex;
  std::deque<RawMatrix*>       _queue;
  boost::detail::atomic_count  _count;
//}

void MatrixMonitor::deposit(RawMatrix* rawMatrix){
    boost::unique_lock<boost::mutex> lock(_mutex);
    _condition.wait(lock, boost::bind(std::less_equal<int>(), boost::ref(_count), max));
    _queue.push_back(rawMatrix);
    ++_count;
    _condition.notify_one();
}

RawMatrix* MatrixMonitor::withdraw(){
    boost::unique_lock<boost::mutex> lock(_mutex);
    _condition.wait(lock, boost::bind(std::greater_equal<int>(), boost::ref(_count), min));
    RawMatrix* elem = _queue.front();
    _queue.pop_front();
    --_count;
    _condition.notify_one();
    return elem;
}

So far I've written the Producer like this:

void MatrixProducer::produce(){
    while(true){
        boost::mutex::scoped_lock lock(_mutex);
        RawMatrix* matrix = rawMatrix();
        _monitor->deposit(matrix);
        boost::this_thread::sleep(boost::posix_time::milliseconds(sleep_msecs));
    }
}
boost::thread& MatrixProducer::start(){
    _thread = boost::thread(boost::bind(&MatrixProducer::produce, this));
    return _thread;
}
RawMatrix* MatrixProducer::rawMatrix(){/*Generates and returns a matrix*/}

The Consumer:

boost::thread& MatrixConsumer::start(){
    _thread = boost::thread(boost::bind(&MatrixConsumer::consume, this));
    return _thread;
}

void MatrixConsumer::consume(){
    while(true){
        boost::mutex::scoped_lock lock(_mutex);
        RawMatrix* matrix = _monitor->withdraw();
        _matrix->deposit(Matrix);
        boost::this_thread::sleep(boost::posix_time::milliseconds(sleep_msecs));
    }
}

Is this okay? Also, who will have the ownership of this: Producer or Consumer and Monitor?

Answer 1

Why do you insist on putting the '' on members. It makes them look so ugly. Also most identifiers beginning with '' are reserved so you need to be careful. If you must use decade old conventions (that have been abandoned) to identify members use 'm_` as the prefix.

The reason I hate them is for this:

--_count;

// That is ugly and harder to read than:

--count; // but that's just an opionon.
         // The bit about being reserved is a problem though.
         // and if you use leading underscores you should understand
         // all the associated rules.

Curious where max/min come from?

_condition.wait(lock, boost::bind(std::less_equal<int>(), boost::ref(_count), max));
                                                                        //   ^^^^^

Passing RAW pointers around is not common in C++ (unlike C).
Normally you wrap a pointer inside a smart pointer object to indicate ownership semantics (and thus who is responsible for deleting the object).

// No indication of ownership is retained or passed the the moniter.
// So we have no idea who should delete rawMatrix?
//
// Have you passed ownership to the moniter or is the caller retaining ownership
// If the owner retains ownership when does he delete it?
void MatrixMonitor::deposit(RawMatrix* rawMatrix){


// I suppose you have to return a pointers as you were passed a pointer.
// but now the caller has no idea weather he should delete the pointer he
// is given or if the pointer is still owned by somebody else.
RawMatrix* MatrixMonitor::withdraw(){

Personally I would change the interface to make sure the ownership symantics are well defined. In this case I would pass ownership to the moniter and pass ownership out to the person withdrawing the item:

void MatrixMonitor::deposit(std::unique_ptr<RawMatrix> rawMatrix){

std::unique_ptr<RawMatrix> MatrixMonitor::withdraw(){

I think you want to separate producer and consumer from the Moniter. These are three different types of object. When you construct the producer/consumer you pass it a monitor object to use when it deposits/withdraws items.

int main()
{
      MatrixMonitor   monitor;
      Producer        farmer(monitor);  // Pass monitor by reference 
                                        // So the farmer is adding to this moiter.
      Consumer        shopper(moniter); // shooper uses the same monitor as the
                                        // farmer but because it is a consumer will
                                        // call withdraw() while the 
                                        // farmer calles deposit()

      farmer.run();
      shopper.run();

      // don't forget to wait for the threads to exit.
      farmer.join();
      shopper.join();
}

Edit: Based on new code for Producer/Consumer

One would assume that the producer needs to finish at some point.
You may want to change while(true) into while(!finished) or similar.

void MatrixProducer::produce(){
    while(true){

Don't see any need for this mutex.

        boost::mutex::scoped_lock lock(_mutex);

Ownership semantics again. Who owns the generated matrix?

        RawMatrix* matrix = rawMatrix();

Answer 2

When I wrote a similar class a few years ago, I found it was actually possible to:

1. Make the collection type templated.
2. Keep the queuing / threading logic in a non-templated base class so that it did not need to be exposed in the header file (unlikely the templated collection it was protecting which did).

Of course it doesn't have to "derive", but separate concerns of the collection type and the locking logic.

The other thing I have always implemented in this kind of collection is multi_push and multi_pop whereby a producer can add more than one item in a single locking action, and a consumer can similarly clear all the data to process rather than pull them off in ones.