HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Thread synchronization with mutex

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
withsynchronizationthreadmutex

Problem

This program prints odd numbers by thread1 and even numbers by thread2 sequentially. Can this code be optimized or made more efficient?

#include 
#include 
#include 

pthread_mutex_t count_mutex = PTHREAD_MUTEX_INITIALIZER;
pthread_cond_t condition_var = PTHREAD_COND_INITIALIZER;

void *function_count1();
void *function_count2();

int count = 1;

int main()
{
    pthread_t thread1, thread2;
    int ret;

    if( (ret=pthread_create( &thread1, NULL, &function_count1, "thread1")) )
    {
        printf("Thread 1 creation failed: %d\n", ret);
        exit (EXIT_FAILURE);
    }

    if( (ret=pthread_create( &thread2, NULL, &function_count2, "thread2")) )
    {
        printf("Thread 2 creation failed: %d\n", ret);
        exit (EXIT_FAILURE);
    }

    pthread_join( thread1, NULL);
    pthread_join( thread2, NULL);

    exit(EXIT_SUCCESS);
}

void *function_count1(void *arg)
{
    while(count <= 10)
    {
        pthread_mutex_lock( &count_mutex );
        if (count % 2 != 0)
        {
            pthread_cond_wait(&condition_var, &count_mutex);
            printf("%s : counter = %d\n", (char *)arg, count++);
        }
        else
        {
            pthread_cond_signal(&condition_var);
        }
        pthread_mutex_unlock( &count_mutex );
    }
}

void *function_count2(void *arg)
{
    while(count <= 10)
    {
        pthread_mutex_lock( &count_mutex );

        if (count % 2 == 0)
        {
            pthread_cond_wait(&condition_var, &count_mutex);
            printf("%s : counter = %d\n", (char *)arg, count++);
        }
        else
        {
            pthread_cond_signal(&condition_var);
        }
        pthread_mutex_unlock(&count_mutex);
    }
}

Solution

-
Since pthread_create() returns an error number on failure, you can put ret into strerror() to get the associated error message instead of just printing the number:

fprintf(stderr, "Thread 1 creation failed: %s\n", strerror(ret));


Notice that I'm calling fprintf() instead of printf(). Since you're printing an error, it must be sent to stderr, not stdin (used by default with printf()).

Also, while you'd normally not need all this code for printing an error, it's necessary since pthread_create() doesn't set errno (which is why ret is useful here).

For code that does set errno (which would be stated on man pages), you'd only need to call perror() to print the corresponding error message:

perror("");


This would also eliminate the need for variables like ret in such code. However, with pthread_create() and related functions, you could probably just put errno in its place and then use perror().

-
The functions called by pthread_create() should still return something. Since the return value won't be useful here, you can just return NULL.

-
There's no need to call exit() right at the end of main(). You're preventing it from returning a value, and it'll always return 0 at this point anyway.

-
This isn't a criticism, but a tip: make sure you're running this through valgrind to check for any memory leaks along the way.

Actually, this may already cause a leak, but I haven't checked to make sure. If it does, then you may need to call both pthread_mutex_destroy() and pthread_cond_destroy() before exiting the program at any point.

Code Snippets

fprintf(stderr, "Thread 1 creation failed: %s\n", strerror(ret));
perror("<insert source of failure here>");

Context

StackExchange Code Review Q#129601, answer score: 5

Revisions (0)

No revisions yet.