up vote 4 down vote favorite
1

This is just a program to show the use of conditional variables when two threads are involved. One thread wants a non-zero value of count, and other thread is responsible for signaling it when the count is non-zero.

Is there something in this code which still needs an improvement? Is there something which could have been done in a better way?

#include <iostream>

/* Declaration of a Mutex variable `mutexA`. */
pthread_mutex_t mutexA;

/* Declaration of a Condition Variable `conditionVariableA`. */
pthread_cond_t  conditionVariableA;

/* `functionA` and `functionB` are the argument functions of the two threads (declared 
below) */
void* functionA (void*);
void* functionB (void*);

/* `count` is the variable shared between threads `A` and `B`. 
Thread `A` wants it to be non zero. Thread `B` will be responsible for making it
non-zero and then issuing a signal. */
int  count = -100;

int main ()
{
    // Declaration of two threads namely `A`, and `B`.
    pthread_t A, B;

    // Initializing the mutex lock to be shared between the threads.
    pthread_mutex_init (&mutexA, NULL);

    /* The function `pthread_cond_init()` initialises the Condition Variable referenced by 
    variable `conditionVariableA` with attributes referenced by variable `attributes`. If 
    `attributes` is NULL, the default condition variable attributes are used. */
    pthread_cond_init (&conditionVariableA, NULL);

    /* Definition of two threads namely A and B */
    pthread_create (&A, NULL, functionA, NULL);
    pthread_create (&B, NULL, functionB, NULL);

    pthread_join (A, NULL);
    pthread_join (B, NULL);
}

void* functionA (void* argA)
{
    while (1)
    {
        pthread_mutex_lock (&mutexA);

        if (count <= 0)
        {
            std :: cout << "\ngnitiaW!\n";
            pthread_cond_wait (&conditionVariableA, &mutexA);   
        }
        else
        {
            // Do something.
            std :: cout << "\nTime to enjoy!\n";
            return 0;
        }
        pthread_mutex_unlock (&mutexA);
    }
    return 0;
}

void* functionB (void* argB)
{
    while (1)
    {
        pthread_mutex_lock  (&mutexA);

        count++;

        if (count > 0)
        {
            pthread_cond_signal (&conditionVariableA);
            std :: cout << "\nSignaled!\n";
            return 0;
        }
        else
        {
            std :: cout << "\nNot signaled yet!";
        }

        pthread_mutex_unlock (&mutexA);
    }
    return 0;
}
share|improve this question
up vote 10 down vote accepted

Couple of issues:

The code is not exception safe.
Any resource that has an open/close symantic should be handled via RAII

{
    pthread_mutex_lock (&mutexA);

    // Code
    // This may throw an exception thus will
    // cause the code to miss the unlock.

    pthread_mutex_unlock (&mutexA);
}

To fix this you should us an RAII locker object.

{
    MutexLocker    lock(mutexA);  // constructor calls lock.
                                  // detructor calls unlock.
    // Code
    // This may throw an exception
    // and it still works correctly.
}

It may work in this simple example:

    if (count <= 0)
    {
        std :: cout << "\ngnitiaW!\n";
        pthread_cond_wait (&conditionVariableA, &mutexA);   
    }

But normally you need to place the wait() inside a loop.

    while (count <= 0)
    {
        std :: cout << "\ngnitiaW!\n";
        pthread_cond_wait (&conditionVariableA, &mutexA);   
    }

If there are multiple threads that could enter this function then between the signal from the producer thread and this thread waking up from the wait another thread could have stolen the object. Thus you need to recheck the state you were waiting for and go back to sleep if it was stolen.

This would have been fixed by the MutexLocker above. But the code as it stands leaves the lock locked.

    else
    {
        // Do something.
        std :: cout << "\nTime to enjoy!\n";

        // You are  returning early without unlock the lock.
        // This causes all your other threads to stall
        // and probably causes deadlock.
        return 0;
    }

    // This point is not reached if you return early.
    pthread_mutex_unlock (&mutexA);
share|improve this answer
up vote 1 down vote

A few years later, C++11 solves all of the problems Loki pointed out in a much cleaner way. Your mutex and condition variable become:

std::mutex mutexA;
std::condition_variable conditionVariableA;

Handling holding a lock through a scope shold use one of the two standard locks, depending on whether you need to unlock it. In functionB(), you don't need to unlock:

while (1)
{
    std::lock_guard<std::mutex> lk(mutexA);

    count++;

    if (count > 0)
    {
        conditionVariableA.notify_one();
        std :: cout << "\nSignaled!\n";
        return 0;
    }
    else
    {
        std :: cout << "\nNot signaled yet!";
    }
}

Whereas in the other, you do, so you use a different lock:

while (1)
{
    std::unique_lock<std::mutex> lk(mutexA);

    if (count <= 0)
    {
        std::cout << "\ngnitiaW!\n";
        conditionVariableA.wait(lk, mutexA);
    }
    else
    {
        // Do something.
        std::cout << "\nTime to enjoy!\n";
        return 0;
    }
}

Threads are easier too:

std::thread A(functionA);
std::thread B(functionB);

A.join();
B.join();

And neither function has to return a void* anymore.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.