Why isn't pthread_cond_signal() being called?

Question

So I am trying to understand pthread_cond_t variables, but the problem is often sometimes pthread_cond_signal()/pthread_cond_broadcast() doesn't work and the sleeping threads are not woken up, leading to a deadlock in my code. Is there a problem in code? What is the better/best way to use condition variables?

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

pthread_mutex_t lock;
pthread_cond_t cv;
int count = 0;
void* routine(){
    pthread_mutex_lock(&lock);
    while(count!=5) pthread_cond_wait(&cv,&lock);
    printf("count value is : %d\n", count);
    pthread_mutex_unlock(&lock);
}

void* routine2(){
    pthread_mutex_lock(&lock);
    for(int i=0; i<7; i++){
        count++;
        printf("%d\n",count);
        if(count == 5) pthread_cond_signal(&cv);
    }
    pthread_mutex_unlock(&lock);
}
int main(){

    pthread_mutex_init(&lock,NULL);
    pthread_cond_init(&cv,NULL);
    pthread_t t1,t2;
    pthread_create(&t1,NULL,&routine,NULL);
    pthread_create(&t2,NULL,&routine2,NULL);

    pthread_join(t1,NULL);
    pthread_join(t2,NULL);

    pthread_mutex_destroy(&lock);
    pthread_cond_destroy(&cv);
}

Answer 1

pthread_cond_wait(cv, mutex) cannot return successfully unless it can obtain the mutex.

However, your waiting thread (t1) will never see the condition it is waiting for (count == 5), because the acting/signaling thread (t2) never releases the lock while the condition is true. Thus, t1 will go right back to waiting.

time   t1           t2            count
----   ---------    -----------   -----
  a     wait...     lock(mutex)    0
  b     wait...      count++       1
  c     wait...      count++       2
     >>>>>several more loops<<<<<<
  f     wait...      count++       5
  g     wait...      signal()      5
  h     wait...      count++       6
  i     wait...      count++       7
  j    (AWAKENED)   unlock(mutex)  7
  k    count != 5?     -           7
  l     wait...        -           7

(The above time table assumes that t1 "goes first," which is not guaranteed. If t2 goes first and obtains the lock first, the table is similar except that t1 is stuck in its initial call to pthread_mutex_lock() until t2 increments count to 7 and then terminates.)

Answer 2

The issue is that you're basing your logic on variables that can change between the time you read the variable and the time that you test the value.

In this code:

    while(count!=5) pthread_cond_wait(&cv,&lock);

If routine() is called first this implicitly means that count is 0 (because routine2() is the only function that changes count, and does not have the lock). routine() will call pthread_cond_wait which will release the mutex lock and then block until the condition is signaled. Note that it also must be able to obtain the mutex lock before it finishes (i.e just the signal alone isn't sufficient). From pthread_cond_wait

Upon successful return, the mutex has been locked and is owned by the calling thread

If routine2() gets the lock first it will iterate until count is 5 and then call pthread_cond_signal. This will not however let routine() continue because the lock is not released at the same time. It will continue iterating through the loop and immediately increment count to 6, before routine() ever gets the chance to read from the count variable. As count cannot possibly be 5 at the time routine() resumes, is will deadlock (get stuck doing the above line forever).

It might seem like you could avoid this by simply not obtaining the lock with routine2():

void* routine2(){
    for(int i=0; i<7; i++){
        count++;
        printf("%d\n",count);
        if(count == 5) {
            pthread_cond_signal(&cv);
        }
    }
}

However there are also problems with this, as there is no guarantee that count will be 5 when routine() is read it as there is nothing stopping routine2() from continuing to process. If that happens routine() will again deadlock as count will have been incremented above 5. This is also unadvisable as accessing a shared variable from more than one thread at a time is classified as nondeterministic behavior.

The change below resolves the issue with a minimal amount of changes. The one major change is to only wait if count is less than 5 (if it's 5 or more the signal was already sent).

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

pthread_mutex_t lock;
pthread_cond_t cv;
int count = 0;
void* routine(){
    pthread_mutex_lock(&lock);
    if (count < 5) {
        pthread_cond_wait(&cv,&lock);
        printf("routine: got signal (%d)\n", count);
    } else {
        printf("routine: signal already sent\n");
    }
    pthread_mutex_unlock(&lock);
}

void* routine2(){
    pthread_mutex_lock(&lock);
    for(int i=0; i<7; i++){
        count++;
        printf("%d\n",count);
        if(count == 5) pthread_cond_signal(&cv);
    }
    pthread_mutex_unlock(&lock);
}

int main(){
    pthread_mutex_init(&lock,NULL);
    pthread_cond_init(&cv,NULL);
    pthread_t t1,t2;
    pthread_create(&t1,NULL,&routine,NULL);
    pthread_create(&t2,NULL,&routine2,NULL);

    pthread_join(t1,NULL);
    pthread_join(t2,NULL);

    pthread_mutex_destroy(&lock);
    pthread_cond_destroy(&cv);
} 

This OnlineGDB snippet demonstrates the potential for deadlock.

Answer 3

You are using the condition variables and mutex correctly but the algorithm is failing. There are two conditions here:

1. t1 must be woken up once count is equal to 5;
2. When count is equal to 5, it must not be changed until t1 is woken up.

Your algorithm is missing the second condition in t2. To make it, set a variable named t1_woken_up to indicate that t1 has been woken up to display the value of counter equal to 5.

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

pthread_mutex_t lock;
pthread_cond_t cv;
int count = 0;
int t1_woken_up = 0;

void* routine(){
    pthread_mutex_lock(&lock);
    while(count!=5) pthread_cond_wait(&cv,&lock);
    printf("t1: count value is %d\n", count);
    printf("t1: Waking up t2\n");
    t1_woken_up = 1;
    pthread_cond_signal(&cv);
    pthread_mutex_unlock(&lock);
}

void* routine2(){
    pthread_mutex_lock(&lock);
    for(int i=0; i<7; i++){
        count++;
        printf("t2: %d\n",count);
        if(count == 5) {
          printf("t2: Waking up t1\n");
          pthread_cond_signal(&cv);
          while (!t1_woken_up) {
            pthread_cond_wait(&cv,&lock);
          }
        }
    }
    pthread_mutex_unlock(&lock);
}

int main(){

    pthread_mutex_init(&lock,NULL);
    pthread_cond_init(&cv,NULL);
    pthread_t t1,t2;
    pthread_create(&t1,NULL,&routine,NULL);
    pthread_create(&t2,NULL,&routine2,NULL);

    pthread_join(t1,NULL);
    pthread_join(t2,NULL);

    pthread_mutex_destroy(&lock);
    pthread_cond_destroy(&cv);
}

Example of execution:

$ gcc t.c -o t -lpthread
$ ./t
t2: 1
t2: 2
t2: 3
t2: 4
t2: 5
t2: Waking up t1
t1: count value is 5
t1: Waking up t2
t2: 6
t2: 7

NB: According to this post and the underlying comments, it is not necessary (better to say "not advised") to set the "volatile" attribute for the shared variables (i.e. the count and t1_woken_up variables).