0

I've clearly developed a flawed understanding of condition variables and how to use them. My intention is to have a single producer and multiple consumer threads but I can demonstrate my problem with a single producer and a single consumer.

There is a single shared variable called work that is protected with a mutex and a condition variable. The producer sets the work variable and signals it is ready, but the consumer thread never get's to run?

If the program works correctly, it should print this line never get's printed, but instead I get consumer never did work...giving up. Any assistance would be greatly appreciated

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

pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;
pthread_cond_t cond = PTHREAD_COND_INITIALIZER;
int work = 0;

void* consumer(void* ptr) {
    pthread_mutex_lock(&mutex);
    while(1) {
        pthread_cond_wait(&cond, &mutex);
        if (work == 0)
            continue;
        printf("this line never get's printed\n");
        work = 0;
        } 
    return NULL;
    }

int main() {
    pthread_t thr;
    pthread_create(&thr, NULL, consumer, NULL);
    sleep(1); /* give consumer moment to lock mutex */

    for(int ndx=0; ndx < 50; ndx++) {
        pthread_mutex_lock(&mutex);
        if (work == 1) {
             printf("consumer never did work...giving up\n");
            return -1;
            }
        work = 1;
        pthread_cond_signal(&cond);
        pthread_mutex_unlock(&mutex);
        }
    return 0;
    }

Compiled with:

$ g++ -pthread simple.cpp -o simple

Running on Debian 7.9 (also reproduced on CentOS 6.7)

$ getconf GNU_LIBPTHREAD_VERSION
NPTL 2.13

$ g++ --version
gcc version 4.7.2 (Debian 4.7.2-5)
user590028
  • 11,364
  • 3
  • 40
  • 57
  • 1
    Your code is C, not C++. It would be advisable to compile it with a C compiler (e.g. `gcc`) instead of with a C++ compiler, and I have in any case removed your c++ tag. – John Bollinger Sep 15 '15 at 14:12
  • 2
    Do not neglect to check the return values of your function calls. If an error occurs, you need to be able to respond appropriately, even if that's only to terminate the program cleanly. – John Bollinger Sep 15 '15 at 14:14
  • Suggestion: read the [Pthread Tutorial](https://computing.llnl.gov/tutorials/pthreads/) from LLNL – Basile Starynkevitch Sep 15 '15 at 14:14
  • `volatile int work = 0;` – JimmyB Sep 15 '15 at 14:15
  • 1
    @JohnBollinger if OP uses a C++ compiler, the code is C++. – mch Sep 15 '15 at 14:15
  • fyi -- i used g++ more as a coding convenience (I can use favorite for loop construct without the need to remember compiler directive). As suggested, I've tried to compile it using pure gcc -- SAME outcome. I also added the volatile keywork -- SAME outcome. FYI -- I've also removed all error checking to made the code simpler to understand -- but obviously I have assiduously done all error checking in actual code. – user590028 Sep 15 '15 at 14:17
  • your example will work if you move the `sleep(1);` into the `for` loop. Your comment tells you the reason. – mch Sep 15 '15 at 14:19
  • Btw, neither `pthread_cond_signal` nor `pthread_mutex_unlock` make any guarantees as to *when* any waiting threads continue. So it may well be that the consumer does not get its chance to run between the producer's `work = 1;` and `if (work == 1)`. – JimmyB Sep 15 '15 at 14:20
  • Hanno -- I was starting to suspect that. So I added `pthread_yield()` to for loop -- and the consumer thread STILL did not get to run. This is completely confusing to me. – user590028 Sep 15 '15 at 14:22
  • Do not try to synchronise threads using `sleep()` or alike. This leads nowhere. – alk Sep 19 '15 at 09:16

2 Answers2

2

Your problem is that although your consumer thread waits for the producer to produce before proceeding, the producer does not likewise wait on the consumer to consume. Just because the producer calls pthread_cond_signal() does not mean that a thread waiting on that condition variable will be scheduled immediately, especially since the producer thread has the associated mutex locked at the time of the call. The consumer cannot resume until it can lock the mutex, and it is entirely plausible that although the producer thread unlocks it, it also cycles the loop and locks it again before the consumer proceeds.

You could use either the same condition variable or another associated with the same mutex to allow consumers to signal the producer that it's ok to proceed.

John Bollinger
  • 160,171
  • 8
  • 81
  • 157
2

Summing up my comments:

1. Data shared between threads should (read: must) always be declared volatile; in your case it should be volatile int work = 0;. (Removed, see comments)

  1. Neither pthread_cond_signal nor pthread_mutex_unlock make any guarantees as to when any waiting threads continue. So it may well be that the consumer does not get its chance to run between the producer's work = 1; and if (work == 1).
JimmyB
  • 12,101
  • 2
  • 28
  • 44
  • But then, wouldn't a `pthread_yield()` in the for loop guarantee the thread get's a chance to run? I tried this -- and still it did not work. – user590028 Sep 15 '15 at 14:23
  • 1
    Nope, `pthread_yield()` does not make any such guarantees either. It's basically at the sole discretion of the OS which thread from which process to run next, or to just return to the calling thread. – JimmyB Sep 15 '15 at 14:24
  • If you yield while you still have the mutex locked, then the producer thread will just be scheduled again, as it is the only one that can make progress. – John Bollinger Sep 15 '15 at 14:24
  • 1
    I have to take issue with the recommendation to declare shared data `volatile`, however. Refer to [this question](http://stackoverflow.com/questions/2484980/why-is-volatile-not-considered-useful-in-multithreaded-c-or-c-programming) and its answers for an extensive discussion of why. – John Bollinger Sep 15 '15 at 14:28
  • 1
    I searched and found in http://stackoverflow.com/a/24138417/1015327 that `pthread_mutex_lock` already includes a (compiler-visible) memory barrier, so `volatile` is really redundant here. – JimmyB Sep 16 '15 at 10:23