How do I sync my two threads which each use a while loop for a shared resource?-CodePudding

I am trying to do this implementation but it's not working properly. I have a global variable called counter which starts at 100 and I have two threads. Both threads are decrementing the counter in a while loop that runs if counter is != 0. However though the thread which does decrement the counter to 0 will stop running as expected. But the thread which does not decrement the counter continues running when it should stop.

How do I fix this?

Below is my code:

int counter = 0;
pthread_mutex_t counter_mutex;

void *Thread1(void *vargs)
{
    while (counter != 0) {
        pthread_mutex_lock(&counter_mutex);
        counter--;
        pthread_mutex_unlock(&counter_mutex);
    }

    sleep(1);
    printf("Completed Thread1\n");
    return NULL;
}

void *Thread2(void *vargs)
{
    while (counter != 0) {
        pthread_mutex_lock(&counter_mutex);
        counter--;
        pthread_mutex_unlock(&counter_mutex);
    }

    sleep(1);
    printf("Completed Thread2\n");
    return NULL;
}

int main(void)
{
    pthread_t tid[2];

    // initialize the mutex
    pthread_mutex_init(&counter_mutex, NULL);

    // create worker threads
    pthread_create(&tid[0], NULL, Thread1, NULL);
    pthread_create(&tid[1], NULL, Thread2, NULL);

    // wait for worker threads to terminate
    pthread_join(tid[0], NULL);
    pthread_join(tid[1], NULL);
    
    // print final counter value
    printf("Counter is %d\n", counter);

    return 0;
}

Output:

Completed Thread1

Thread1 completes but the program runs indefinitely because Thread2 stays in the while loop and doesn't finish.

Or vice versa, where Thread2 completes and then runs indefinitely because Thread1 stays 
in the while loop and doesn't finish.

I'm really confused on how to approach fixing this problem because the two Threads should be running and stopping when counter == 0. However only the Thread that decrements counter to 0, stops while the other runs indefinitely.

Any and all help is really appreciated!

Thank you so much

CodePudding user response：

At some point, while one thread will be blocked waiting to lock the mutex, the other will have decremented counter to zero. As soon as the waiting thread gains access to the lock, it will decrement as well, resulting in -1. counter will never approach zero again, and it will be decremented until Undefined Behavior is invoked by overflowing a signed integer.

None of this really matters, because the read of counter in each while loop predicate is not protected by the mutex

while (counter != 0)

which means you can have a read/write race condition.

Instead, structure your locks so they fully surround all reads & writes, and adjust your predicate to be independently checked.

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

int counter = 0;
pthread_mutex_t counter_mutex;

void *runner(void *arg) {
    int *n = arg;
    int done = 0;

    while (!done) {
        pthread_mutex_lock(&counter_mutex);

        if (counter == 0)
            done = 1;
        else
            counter--;

        pthread_mutex_unlock(&counter_mutex);
    }

    printf("Completed Thread %d\n", *n);

    return NULL;
}

int main(void)
{
    pthread_t tid[2];
    int args[2] = { 1, 2 };

    pthread_mutex_init(&counter_mutex, NULL);
    pthread_create(&tid[0], NULL, runner, &args[0]);
    pthread_create(&tid[1], NULL, runner, &args[1]);
    pthread_join(tid[0], NULL);
    pthread_join(tid[1], NULL);

    printf("Counter is %d\n", counter);

    return 0;
}

CodePudding user response：

FYI: This is practically always a bad idea:

while (...trivial condition...) {
    pthread_mutex_lock(&some_mutex);

    ...do some work...

    pthread_mutex_unlock(&some_mutex);
}

The reason it's bad is that the loop tries to keep the mutex locked almost 100% of the time. The only time when the mutex is not locked is the brief moment when the loop evaluates the "...trivial condition..."

There's no point in executing the loop in more than one thread at the same time because the mutex prevents more than one thread from ever doing "...work..." at the same time.

If you're trying to use threads for parallel computing, then something like this works better:

typedef struct { ... } task_t;

int find_a_task(task_t* task) {
    int result = FALSE;

    pthread_mutex_lock(&some_mutex);
    if (...there's more work to be done...) {
        ...copy from shared data into *task...
        result = TRUE;
    }
    pthread_mutex_unlock(&some_mutex);

    return result;
}

task_t local_task;
while (find_a_task(&local_task)) {

    do_some_heavy_work_on(&local_task);

    pthread_mutex_lock(&some_mutex);
    if (...contents of local_task still are meaningful...) {
        copy from local_task back to shared data structure
    }
    pthread_mutex_unlock(&some_mutex);
}

The idea is, to do most of the heavy work without keeping any mutex locked. The mutex is only briefly locked (a) before doing the heavy work, to copy shared data into private, local variables and (b) after the heavy work, to copy results, if still valid,* back into the shared data.

* The result might not still be valid because of some other thread changing shared data items that the caller used. This is optimistic locking. It may sound inefficient, but in many programs, the efficiency gained by not keeping a mutex locked while doing heavy work is MUCH greater than the efficiency lost because of threads occasionally duplicating or invalidating each other's effort.