patterncppModerate
Maintaining a count with two threads using conditional variables
Viewed 0 times
withthreadsconditionaltwousingvariablescountmaintaining
Problem
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
Is there something in this code which still needs an improvement? Is there something which could have been done in a better way?
``
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)
{
pthread_cond_signal (&conditionVariableA);
std :: cout << "\nSignaled!\n";
return 0;
}
else
{
std :: cout << "\nNot signaled yet!";
}
pthread_mutex_unlock (&mutexA);
}
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
/ 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)
{
pthread_cond_signal (&conditionVariableA);
std :: cout << "\nSignaled!\n";
return 0;
}
else
{
std :: cout << "\nNot signaled yet!";
}
pthread_mutex_unlock (&mutexA);
}
Solution
Couple of issues:
The code is not exception safe.
Any resource that has an open/close symantic should be handled via RAII
To fix this you should us an RAII locker object.
It may work in this simple example:
But normally you need to place the wait() inside a loop.
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.
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);Code Snippets
{
pthread_mutex_lock (&mutexA);
// Code
// This may throw an exception thus will
// cause the code to miss the unlock.
pthread_mutex_unlock (&mutexA);
}{
MutexLocker lock(mutexA); // constructor calls lock.
// detructor calls unlock.
// Code
// This may throw an exception
// and it still works correctly.
}if (count <= 0)
{
std :: cout << "\ngnitiaW!\n";
pthread_cond_wait (&conditionVariableA, &mutexA);
}while (count <= 0)
{
std :: cout << "\ngnitiaW!\n";
pthread_cond_wait (&conditionVariableA, &mutexA);
}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);Context
StackExchange Code Review Q#14214, answer score: 10
Revisions (0)
No revisions yet.