Bug in multithreaded program

Question

I have to make a program which has a shared variable (counter with initial value = 35) and 5 threads. I have to make the program so that each thread accesses the value of the counter and decrements it by 1. This should continue until counter = 0.

This is what I have for the code so far, but the problem is that only one thread is decrementing the value of counter to 0.

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

#define NTHREADS 5
void *thread_function1(void *);
void *thread_function2(void *);
void *thread_function3(void *);
void *thread_function4(void *);
void *thread_function5(void *);
pthread_mutex_t mutex1 = PTHREAD_MUTEX_INITIALIZER;
short int  counter = 35;


main()
{
   pthread_t thread_id[NTHREADS];
   pthread_t thread1, thread2, thread3, thread4, thread5;
int status, status2, status3, status4, status5;

status = pthread_create(&thread1, NULL, thread_function1, NULL);
if(status!=0){
    fprintf(stderr,"thread 1 failed\n");    
}

status2 = pthread_create(&thread2, NULL, thread_function2, NULL);
if(status2!=0){
    fprintf(stderr,"thread 2 failed\n");    
}

status3 = pthread_create(&thread3, NULL, thread_function3, NULL);
if(status3!=0){
    fprintf(stderr,"thread 3 failed\n");    
}

status4 = pthread_create(&thread4, NULL, thread_function4, NULL);
if(status4!=0){
    printf("thread 4 failed\n");    
}

status5 = pthread_create(&thread5, NULL, thread_function5, NULL);
if(status5!=0){
    fprintf(stderr,"thread 5 failed\n");    
}

//pthread_join(thread1, NULL);
//int x = counter;

printf("created all the threads \n");

printf("joining thread 1");
    pthread_join(thread1, NULL);
printf("joining thread 2");
    pthread_join(thread2, NULL);
printf("joining thread 3");
    pthread_join(thread3, NULL);
printf("joining thread 4");
    pthread_join(thread4, NULL);
printf("joining thread 5");
    pthread_join(thread5, NULL);                          

   printf("Final counter value: %d\n", counter);
}

void *thread_function1(void *dummyPtr)
{
   printf("Thread number %ld\n", pthread_self());   

while(counter>0){
   srand(time(NULL));

   int r = rand()%3;
  printf(" %d\n", r);
   pthread_mutex_lock( &mutex1 );
  printf("entered the mutex");
   counter--;
   printf(" %d\n", counter);
   sleep(r);
   pthread_mutex_unlock( &mutex1 );
   printf("mutex unlocked");
   pthread_yield();
}

}

void *thread_function2(void *dummyPtr)
{
   printf("Thread number %ld\n", pthread_self());   

while(counter>0){
   srand(time(NULL));

   int r = rand()%3;
  printf(" %d\n", r);
   pthread_mutex_lock( &mutex1 );
  printf("entered the mutex");
   counter--;
   printf(" %d\n", counter);
   sleep(r);
   pthread_mutex_unlock( &mutex1 );
   pthread_yield();
}

}

void *thread_function3(void *dummyPtr)
{
   printf("Thread number %ld\n", pthread_self());   

while(counter>0){
   srand(time(NULL));
   int r = rand()%3;
  printf(" %d\n", r);
   pthread_mutex_lock( &mutex1 );
  printf("entered the mutex");
   counter--;
   printf(" %d\n", counter);
   sleep(r);
   pthread_mutex_unlock( &mutex1 );
   pthread_yield();
}

}

void *thread_function4(void *dummyPtr)
{
   printf("Thread number %ld\n", pthread_self());   

while(counter>0){
   srand(time(NULL));

   int r = rand()%3;
  printf(" %d\n", r);
   pthread_mutex_lock( &mutex1 );
  printf("entered the mutex");
   counter--;
   printf(" %d\n", counter);
   sleep(r);
   pthread_mutex_unlock( &mutex1 );
   pthread_yield();
}

}

void *thread_function5(void *dummyPtr)
{
   printf("Thread number %ld\n", pthread_self());   

while(counter>0){
   srand(time(NULL));

   int r = rand()%3;
   printf(" %d\n", r);
   pthread_mutex_lock( &mutex1 );
   printf("entered the mutex");
   counter--;
   printf(" %d\n", counter);
   sleep(r);
   pthread_mutex_unlock( &mutex1 );
   pthread_yield();
}

}

Can anyone please help ? Thanks

Answer 1

While Antti found a good one, I have a few more comments to make:

• you don't need five identical functions. You can have only one function, and start it five times as different threads.

• you are sleeping with the mutex locked. That means that the other threads will block while a thread sleeps. I think you want to sleep after releasing the lock, so that other threads can have at the counter.

• You are reading the counter outside of mutex protection when you check the counter in the while loop's condition. You need to lock the mutex when you access the shared resource, no matter if you are reading or writing. If you don't then you'll have race conditions.

• if you follow my advice and move the sleep after the mutex is released, then calling pthread_yield is not necessary, since a sleep also yields.

Answer 2

int r = rand()%3;
/* ... */
sleep(rand);

r is a random number between 0 and 2, but you sleep rand seconds, which is the address of a function, which will be implicitly casted to an unsigned int - so the thread will sleep for a very very long time. Use sleep(r); instead.

Also, note that you read counter while not holding the mutex (in while(counter > 0)), which may lead to the program working incorrectly depending on architecture, caching and compiler optimizations. You should lock the mutex, read the value of counter to a local variable, then unlock the mutex and check whether the value is positive.