current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 0 down vote favorite

This is just a program to show the use of condition variable 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

closed as off-topic by Jamal♦ Dec 20 '14 at 5:07

This question appears to be off-topic. The users who voted to close gave this specific reason:

If this question can be reworded to fit the rules in the help center, please edit the question.

1 Answer 1

up vote 1 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
    
Very thankful for your suggestions. I will surely remember next time to make the code exception safe. –  TheIndependentAquarius Aug 13 '12 at 8:40

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