I am using the following code to to implement a basic prod-consumer problem in C. On rare occasions though, i'm getting wrong outputs such as mentioned below.
The Received value reaches 0, but when it reaches 0 when there are still producer cycles to be left, then the receiver keeps getting 0 values and the output goes awry.
Please help me understand what is wrong with my code. Thanks.
C CODE:
#include <pthread.h>
#include <stdio.h>
#include <semaphore.h>
sem_t semaphore;
pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;
int queue[50];
int queueLength;
void *producer( void * param)
{
for ( int i=0; i<50; i++ )
{
// Add item to queue
pthread_mutex_lock( &mutex );
queue[ queueLength++ ] = i;
printf("Sent %d\n", i);
pthread_mutex_unlock( &mutex );
// Signal semaphore
sem_post( &semaphore );
}
}
void *consumer(void * param)
{
for ( int i=0; i<50; i++ )
{
int item;
// Wait if nothing in queue
if (queueLength<1) { sem_wait(&semaphore); }
pthread_mutex_lock( &mutex );
item = queue[ -- queueLength ];
pthread_mutex_unlock( &mutex );
printf("Received %i\n", item);
}
}
int main()
{
pthread_t threads[2];
sem_init( &semaphore, 0, 1 );
pthread_create( &threads[0], 0, producer, 0 );
pthread_create( &threads[1], 0, consumer, 0 );
pthread_join( threads[0], 0 );
pthread_join( threads[1], 0 );
sem_destroy( &semaphore );
}
Wrong Output:
Sent 0
Sent 1
Sent 2
Sent 3
Sent 4
Sent 5
Sent 6
Sent 7
Sent 8
Sent 9
Sent 10
Sent 11
Sent 12
Sent 13
Sent 14
Sent 15
Sent 16
Sent 17
Sent 18
Sent 19
Sent 20
Sent 21
Sent 22
Sent 23
Sent 24
Sent 25
Sent 26
Sent 27
Sent 28
Received 28
Received 27
Received 26
Received 25
Received 24
Received 23
Received 22
Received 21
Received 20
Received 19
Received 18
Received 17
Received 16
Received 15
Received 14
Received 13
Received 12
Received 11
Received 10
Received 9
Received 8
Received 7
Received 6
Received 5
Received 4
Received 3
Received 2
Received 1
Received 0
Received 0
Received 0
Received 0
Received 0
Received 0
Received 0
Received 0
Received -8
Received 0
Received 0
Received 0
Received 0
Received 0
Received 0
Received 0
Received 0
Received 0
Received 0
Received 0
Received 0
Received 1
Sent 29
Sent 30
Sent 31
Sent 32
Sent 33
Sent 34
Sent 35
Sent 36
Sent 37
Sent 38
Sent 39
Sent 40
Sent 41
Sent 42
Sent 43
Sent 44
Sent 45
Sent 46
Sent 47
Sent 48
Sent 49
By the time your consumer thread starts, semaphore has already been signaled, and queueLength is larger than zero. Your consumer thread will immediately start dequeueing (since queueLength isn't zero), until it reaches zero.
As soon as the queue is depleted, in the next consumer loop iteration (with queueLength == 0), it will enter the if clause and try to wait on the semaphore (which has been already signaled), and acquire the lock immediately.
So the semaphore cannot be inside the if condition, meaning you could use something like:
for ( int i=0; i<50; i++ )
{
int item;
// wait for signal
sem_wait(&semaphore);
pthread_mutex_lock( &mutex );
item = queue[--queueLength];
pthread_mutex_unlock( &mutex );
printf("Received %i\n", item);
}
My suggestion (if this is your actual problem) would be to create a simple circular buffer instead. If implemented properly (i.e. only a single producer, single consumer, if the producer only moves the head, and the consumer only moves the tail, and presuming both variables are int or smaller -- i.e. can be read and written atomically), you won't need the mutex at all (only the semaphore for signalization).
The array named queue in your example is actually a stack (LIFO), which is unusual in producer/consumer patterns where you usually want to dequeue and process oldest items first.