My consumer and producer threads aren't synchronizing correctly?

[samalex1014]

Hey, so I have a school assignment to create a program that uses a producer thread to create a series of random numbers, and a consumer thread to read those numbers (via a shared bounded buffer), and record the min, max, average, and number of items. Now, the threads aren't synching well. Here's my code:

#include <pthread.h>
#include <stdlib.h>
#include <stdio.h>

pthread_cond_t empty;
pthread_cond_t full;
int done = 0;
pthread_mutex_t lock;
int in = 0;
int out = 0;
int BUFFER_SIZE = 5;
int buffer[5];

void *consumer();
void *producer();

int main() {
    pthread_t tidC;
    pthread_t tidP;

    pthread_cond_init(&empty, NULL);
    pthread_cond_init(&full, NULL);

    pthread_create(&tidP, NULL, &producer, NULL);
    pthread_create(&tidC, NULL, &consumer, NULL);

    pthread_join(tidC, NULL);
    pthread_join(tidP, NULL);

    return 0;
}

void * producer() {
    int seed = 6;
    int reps = 7;
    int num = 0;
    int i = 0;

    srand(seed);
    printf("Producer in for\n");/*DEBUG*/    
    for(i; i<reps; i++) {
        printf("Producer making item %d\n", i);
        num = rand();
    
        pthread_mutex_lock(&lock);

        while(pthread_cond_signal(&full))
        {
            pthread_cond_wait(&empty, &lock);
        }
    
        buffer[in] = num;


        pthread_mutex_unlock(&lock);/*exiting critical section*/

        pthread_cond_signal(&full);

        in++;

        if(in == BUFFER_SIZE) {
            in = 0;
        }
    }

    done = 1;
}

void * consumer() {
    int num = 0;
    int min=0;
    int max=0;
    int avg=0;
    int numItems=0;
    int first=1;
    int reps = 3;
    int sum = 0;    

    printf("Consumer Entering While\n");/*DEBUG*/

    while(!done) {
    
        printf("Consumer reading item %d\n", numItems);

        pthread_mutex_lock(&lock);

        while(pthread_cond_signal(&empty)){
            pthread_cond_wait(&full, &lock);    
        }

        num = buffer[out];

        pthread_mutex_unlock(&lock); /*exit critical section*/    

        pthread_cond_signal(&empty);

        out++;

        if(out == BUFFER_SIZE)
            out = 0;

        /*processing*/

        if(first) {
            min = num;
            max = num;
            sum = num;
            first = 0;
            numItems = 1;
        }
        else {
            if(num < min)
                min = num;

            sum =+ num;

            if(num>max)
                max = num;

            numItems++;
        }
    }

    
    avg = sum/numItems;/*calc avg*/

    /*report stats*/
    printf("Minimum: %d\n", min);
    printf("Maximum: %d\n", max);
    printf("Average: %d\n", avg);
    printf("Items Produced: %d\n", numItems);
}

And output:

Producer in for
Consumer Entering While
Consumer reading item 0
Producer making item 0
Consumer reading item 1
Consumer reading item 2
Consumer reading item 3
Producer making item 1
Producer making item 2
Producer making item 3
Producer making item 4
Producer making item 5
Producer making item 6
Consumer reading item 4
Minimum: 0
Maximum: 4978
Average: 995
Items Produced: 5

Any advice?

[tabstop]

Well, don't print "Consumer reading item n" until the consumer is actually reading item n (ie after you've got the signal to read something). And I suspect that if your producer makes a stack of numbers before your consumer gets a chance to read them all, you may be dropping numbers at the end.

[anduril462]

Adding more debugging output, between each step would help. For example, when you produce/consume a number, print the index (i), the value (num) and how many items are currently in the buffer (you need to add such a variable). Also, checking all error conditions and printing useful error messages would be good.

Read the man pages, you seem to be misunderstanding pthread_cond_signal and pthread_cond_wait.

pthread_cond_wait blocks the current thread until the specified condition is signaled.
pthread_cond_signal sends a signal to unblock the thread waiting on said signal.

Don't signal in a loop, only signal once each time the buffer is full/empty. Don't wait in a loop, only wait once every time the buffer is full/empty. Also, you don't want to wait indiscriminately every time through your main producer/consumer loops, as you will encounter a deadlock. Only wait for the empty signal if the buffer is full. I added a count variable to help here. Also, define buffer in terms of BUFFER_SIZE, so if you change BUFFER_SIZE, buffer changes too. Your producer logic should be something like this (consumer looks similar):

producer
    for i from 0 to reps
        lock mutex
        if count is BUFFER_SIZE
            wait empty
        generate number
        add to buffer
        increase count
        if count is BUFFER_SIZE
            signal full
        unlock mutex

Note, I don't think your implementation is ideal. If the buffer fills all the way up, your producer must wait for it to empty completely before starting to fill it again. Try thinking in terms of
if the buffer is not full, produce a new number
and
if the buffer is not empty, consume a number
Switch your conditions to not_full and not_empty, and adjust the logic accordingly. You should then always produce as long as there is room in the buffer, and always consume as long as there is something in the buffer to consume.

Also, the done flag alone is not a good way to stop the consumer. If the producer produces the last item, and sets done, but the consumer is behind by several items, it may not empty the buffer before the loop terminates. Try:

while (!done || count > 0)

[samalex1014]

Thanks! A lot of the implementation is based on the specs the teacher gave out. The buffer is meant to act as an infinite buffer, which is handled by the if in == buffersize set in to 0, so that when it reaches the end of the buffer, it returns back to the beginning. Theoretically, the program should work with the producer making an item, signaling to the consumer the item is made, then the consumer reading that item, and signaling to the producer it was read, and so on and so forth. The done flag was also specified by the instructor. But I will go in and change it to your recommendations, and let you know how that works.

[anduril462]

"Infinite" is not the best word, since it is limited to BUFFER_SIZE spaces. Circular buffer is the more appropriate term. You shouldn't need to signal "I made something" every time the producer produces a single number, it's excessive and will cause unnecessary signals to be sent. Think of it in the abstract, i.e. take the computer and programming out of it. Imagine one person (Alice) putting things (chocolate chip cookies?) on a conveyor belt (circular buffer), and another (Bob) taking them off.

Under normal circumstances, Alice puts some cookies on the belt when they come out of the oven, and Bob takes some off and puts them in boxes. They each work in batches at slightly different rates, sometimes Alice puts a bunch of cookies on at once, sometimes Bob takes off a bunch at once. So the belt may be a little more or less full at a particular time.

Now, imagine Bob ran out of boxes, but Alice keeps taking batches out of the oven. Eventually the belt fills up (over-produced, buffer is full) and she must wait until there is more space on the belt (wait for a not_full signal). Once Bob gets more boxes, he can start pulling cookies off the belt and tell Alice there is room, i.e. he can signal the belt is not_full.

In the reverse case (over-consumed), the belt (buffer) is empty. Bob has been boxing things up too fast, and Alice doesn't have the new batch of cookies ready. Bob must wait until there are cookies on the belt (wait for a not_empty signal). Once Alice produces some cookies and puts them on the belt, she will signal not_empty, and Bob can start boxing up again.

If you use full and empty signals, Alice could not start putting cookies on the belt until Bob boxed up all the cookies on a full belt. That is a waste of Alice's time, she could be producing some cookies once there is free space on the belt. Similarly, Bob could not take cookies off the belt until Alice filled it up completely, wasting Bob's time (who could be boxing up Alice's first batch before the second, third, etc are done). In either case, it is inefficient, and can cause your system to oscillate between completely full and completely empty with no real in between state, where Alice and Bob might be working at the same pace, and there is always, say, 3 batches of cookies on the belt. You lose much of the efficiency of making this multi-threaded.

EDIT:
Much of this sounds like it's about the name of your condition variables -- which it is to a degree -- but more importantly, it's about how you implement them and what they truly represent. You could name them foo_cond and bar_cond, as long as they wait and signal properly, i.e. signalling not_full and not_empty.