What are the problems with this producer/consumer implementation?

Question

So I'm looking at using a simple producer/consumer queue in C++. I'll end up using boost for threading but this example is just using pthreads. I'll also end up using a far more OO approach, but I think that would obscure the details I'm interested in at the moment.

Anyway the particular issues I'm worried about are

1. Since this code is using push_back and pop_front of std::deque - it's probably doing allocation and deallocation of the underlying data in different threads - I believe this is bad (undefined behaviour) - what's the easiest way to avoid this?
2. Nothing is marked volatile. But the important bits are mutex protected. Do I need to mark anything as volatile and if so what? - I don't think I do as I believe the mutex contains appropriate memory barriers etc., but I'm unsure.

Are there any other glaring issues?

Anyway heres the code:

#include <pthread.h>
#include <deque>
#include <iostream>

struct Data
{
  std::deque<int> * q;
  pthread_mutex_t * mutex;
};

void* producer( void* arg )
{
  std::deque<int> &q = *(static_cast<Data*>(arg)->q);
  pthread_mutex_t * m =  (static_cast<Data*>(arg)->mutex);

  for(unsigned int i=0; i<100; ++i)
  {
    pthread_mutex_lock( m );
    q.push_back( i );
    std::cout<<"Producing "<<i<<std::endl;
    pthread_mutex_unlock( m );
  }
  return NULL;
}

void* consumer( void * arg )
{
  std::deque<int> &q = *(static_cast<Data*>(arg)->q);
  pthread_mutex_t * m =  (static_cast<Data*>(arg)->mutex);

  for(unsigned int i=0; i<100; ++i)
  {
    pthread_mutex_lock( m );
    int v = q.front();
    q.pop_front();
    std::cout<<"Consuming "<<v<<std::endl;
    pthread_mutex_unlock( m );
  }  
  return NULL;
}

int main()
{
  Data d;

  std::deque<int> q;
  d.q = &q;

  pthread_mutex_t mutex;
  pthread_mutex_init( &mutex, NULL );
  d.mutex = & mutex;

  pthread_t producer_thread;
  pthread_t consumer_thread;

  pthread_create( &producer_thread, NULL, producer, &d );
  pthread_create( &consumer_thread, NULL, consumer, &d );

  pthread_join( producer_thread, NULL );
  pthread_join( consumer_thread, NULL );
}

EDIT:

I did end up throwing away this implementation, I'm now using a modified version of the code from here by Anthony Williams. My modified version can be found here This modified version uses a more sensible condition variable based approach.

Answer 1

Since this code is using push_back and pop_front of std::deque - it's probably doing allocation and deallocation of the underlying data in different threads - I believe this is bad (undefined behaviour) - what's the easiest way to avoid this?

As long as only one thread can modify the container at a time, this is okay.

Nothing is marked volatile. But the important bits are mutex protected. Do I need to mark anything as volatile and if so what? - I don't think I do as I believe the mutex contains appropriate memory barriers etc., but I'm unsure.

So long as you correctly control access to the container using a mutex, it does not need to be volatile (this is dependent upon your threads library, but it wouldn't be a very good mutex if it didn't provide a correct memory barrier).

Answer 2

1. It is perfectly valid to allocate memory in one thread and free it in another if both threads are in the same process.

2. Using a mutex to protect access to the deque should provide the correct memory access configuration.

EDIT: The only other thing to think about is the nature of the producer and consumer. Your synthesized example lacks some of the subtleties involved with a real implementation. For example, how will you synchronize the producer with the consumer if they are not operating at the exact same rate? You might want to consider using something like a pipe or an OS queue instead of a deque so that the consumer can block on read if there is no data ready to process.