Using of shared variable by 10 pthreads

Question

The problem is in following:

I want to write a short program that creates 10 threads and each prints a tread "id" that is passed to thread function by pointer.

Full code of the program is below:

#include <pthread.h>
#include <stdio.h>
#include <stdlib.h>

struct params {
        pthread_mutex_t mutex;
        int id;
};

typedef struct params params_t;

void* hello(void* arg){
    int id;
    pthread_mutex_lock(&(*(params_t*)(arg)).mutex);
    id = (*(params_t*)(arg)).id;
    pthread_mutex_unlock(&(*(params_t*)(arg)).mutex);
    printf("Hello from %d\n", id);
}


int main() {
    pthread_t threads[10];
    params_t params;
    pthread_mutex_init (&params.mutex , NULL);

    int i;
    for(i = 0; i < 10; i++) {
            params.id = i;
            if(pthread_create(&threads[i], NULL, hello, &params));
    }

    for(i = 0; i < 10; i++) {
            pthread_join(threads[i], NULL);
    }

    return 0;
}

The supposed output is (not necessary in this order):

Hello from 0
....
Hello from 9

Actual result is:

Hello from 2
Hello from 3
Hello from 3
Hello from 4
Hello from 5
Hello from 6
Hello from 8
Hello from 9
Hello from 9
Hello from 9

I tried to place mutex in different places in hello() function, but it didn't help.

How should I implement thread sync?

EDIT: Supposed result is not necessary 0...9 it can be any combination of these numbers, but each one should appear only one time.

Solution 1

There are two problems:

A. You're using a lock but main is unaware of this lock.

B. A lock is not enough in this case. What you would want is for threads to cooperate by signalling each other (because you want main to not increment the variable until a thread says that it is done printing it). You can use a pthread_cond_t to achieve this (Look here to learn more about this). This boils down to the following code (basically, I added an appropriate usage of pthread_cond_t to your code, and a bunch of comments explaining what is going on):

#include <pthread.h>
#include <stdio.h>
#include <stdlib.h>

struct params {
        pthread_mutex_t mutex;
        pthread_cond_t done;
        int id;
};

typedef struct params params_t;

void* hello(void* arg){

    int id;
    /* Lock.  */
    pthread_mutex_lock(&(*(params_t*)(arg)).mutex);

    /* Work.  */
    id = (*(params_t*)(arg)).id;
    printf("Hello from %d\n", id);

    /* Unlock and signal completion.  */
    pthread_mutex_unlock(&(*(params_t*)(arg)).mutex);
    pthread_cond_signal (&(*(params_t*)(arg)).done);

    /* After signalling `main`, the thread could actually
    go on to do more work in parallel.  */
}


int main() {

    pthread_t threads[10];
    params_t params;
    pthread_mutex_init (&params.mutex , NULL);
    pthread_cond_init (&params.done, NULL);

    /* Obtain a lock on the parameter.  */
    pthread_mutex_lock (&params.mutex);

    int i;
    for(i = 0; i < 10; i++) {

            /* Change the parameter (I own it).  */    
            params.id = i;

            /* Spawn a thread.  */
            pthread_create(&threads[i], NULL, hello, &params);

            /* Give up the lock, wait till thread is 'done',
            then reacquire the lock.  */
            pthread_cond_wait (&params.done, &params.mutex);
    }

    for(i = 0; i < 10; i++) {
            pthread_join(threads[i], NULL);
    }

    /* Destroy all synchronization primitives.  */    
    pthread_mutex_destroy (&params.mutex);
    pthread_cond_destroy (&params.done);

    return 0;
}

I see that the example you are trying is a toy program to probably learn about the POSIX thread library. In the real world, as we all know this can be done much faster without even using threads. But you already know this.

Solution 2

The problem lies in the below code:

for(i = 0; i < 10; i++) 
{             
  params.id = i;             
 if(pthread_create(&threads[i], NULL, hello, &params));     
} 

Your params.id value keeps getting updated in the main thread, whereas you are passing the same pointer to all the threads.

Please create seperate memory for params by dynamically allocating it and pass it to different threads to solve the problem.

EDIT1: Your usage of mutex to protect is also an incorrect idea. Though your mutex if used in main while setting the id also, may make the updation mutually exclusive, but you may not get your desired output. Instead of getting values from 0 .. 9 in different threads, you may get all 9s or still multiple threads may print same values.

So, using thread synchronization is not such a good idea for the output which you are expecting. If you still need to use one param variable between all threads and get output as 0 to 9 from each of the threads, better move the pthread_join into the first loop. This will ensure that each thread gets created, prints the value and then returns before the main spawns the next thread. In this case, you don't need the mutex also.

EDIT2: As for the updated question, where it is asked that it is not necessary to print the numbers 0..9 in a sequence, the printing can be random, but only once, the problem still remains the same more or less.

Now, let's say, the value of params.id is first 0 and thread 0 got created, now, thread 0 must print it before it is updated in the main thread, else, when thread 0 accessess it, the value of params.id would have become 1 and you will never get your unique set of values. So, how to ensure that thread 0 prints it before it is updated in main, Two ways for it:

• Ensure thread 0 completes execution and printing before main updates the value
• Use condition variables & signalling to ensure that main thread waits for thread 0 to complete printing before it updates the value (Refer to Arjun's answer below for more details)

In my honest opinion, you have selected the wrong problem for learning synchronization & shared memory. You can try this with some good problems like "Producer-Consumer", where you really need synchronization for things to work.

Solution 3

The problem is that you are modifying the params.id "unprotected" in main. This modification in main also needs to be mutex protected. You could protect this access by localizing this by creating getId() and setId() functions that would lock the mutex and protect access to the id, as follows. This will most likely still give the problem reported, since depending on when the thread calls getData() it will have one value or another. So to solve this, you could add an incrementId() function and call it from the hello() function.

struct params {
        pthread_mutex_t mutex;
        int id;
};

typedef struct params params_t;

int getId(params_t *p)
{
    int id;
    pthread_mutex_lock(&(p->mutex));
    id = p->id;
    pthread_mutex_unlock(&(p->mutex));

    return id;

}

void setId(params_t *p, int val)
{
    pthread_mutex_lock(&(p->mutex));
    p->id = val;
    pthread_mutex_unlock(&(p->mutex));
}

void incrementId(params_t *p)
{
    pthread_mutex_lock(&(p->mutex));
    p->id++;
    pthread_mutex_unlock(&(p->mutex));
}

void* hello(void* arg){
    params_t *p = (params_t*)(arg);
    incrementId(p);
    int id = getId(p);

    // This could possibly be quite messy since it
    // could print the data for multiple threads at once
    printf("Hello from %d\n", id);
}


int main() {
    pthread_t threads[10];
    params_t params;
    params.id = 0;
    pthread_mutex_init (&params.mutex , NULL);

    int i;
    for(i = 0; i < 10; i++) {
            if(pthread_create(&threads[i], NULL, hello, &params));
    }

    for(i = 0; i < 10; i++) {
            pthread_join(threads[i], NULL);
    }

    return 0;
}

A better way to get a unique thread id would be to define the hello method as follows:

void* hello(void* arg){
    pthread_t threadId = pthread_self();
    printf("Hello from %d\n", threadId);
}

And to avoid the problem with all threads trying to print at once, you could do the following:

void* hello(void* arg){
    params_t *p = (params_t*)(arg);
    pthread_mutex_lock(&(p->mutex));

    p->id++;
    int id = p->id;
    printf("Hello from %d\n", id);

    pthread_mutex_unlock(&(p->mutex));
}

Comments

[Jay]

Even with the usage suggested by you, the OP might still get the same results which he is getting now. Am I missing something?

[Brady]

@Jay, I updated the code to better show the usage. But you're right, each thread wont have its own value, and the value they have depends when they call getId()

[Alex]

I could use the separate memory, but the main idea was in shared variable. Anyway, thanks.

[ArjunShankar]

@Jay - This is a good answer. However, there is a way to do this correctly without using a 'join' in loop, and without using separate memory. i.e. use a pthread_cond_t. While this is a toy program, signalling between threads using a condition variable seems to me to the right way for this toy program (and the OP gets to learn something new). Check out my answer.

[Alex]

@ArjunShankar regarding to the program it's a really toy program in order to understand how do pthreads work.

[Brady]

@Jay, I figured out a better way to do it without having to create a params instance per thread: just increment the id from within the thread.

[Brady]

This would work for short lived threads, but not if the OP wants the threads to last longer. And the new threads wont be created until the previous threads complete.

[Alexandru C.]

I just read the question. The solution applies for the current statement of the problem. It's not meant to be generic.

[Alex]

@AlexandruC. In this case you're right, it will work and it is the simplest solution, but in this case I even don't need an array of pthread_t's. I only need a one variable with this type.

[Alexandru C.]

@Alex If you want to print the ids in order 0..9 then you would need a synchronization similar to the one in my example, although maybe not waiting for the thread to finish. Either way, you lose parallelism. The threads have to wait for each other, in the order of their creation.

[ArjunShankar]

@Alex - Like I said, you need more than just a lock to make this work correctly while still using shared memory. You really should read my answer. However, again, like I said, what Jay says about separate memory is a fair point. When you write real world programs, you'll know which to use when.

[Jay]

@Arjun, you answer is correct, but appeared a little complicated for a Toy Progam. But, from the perspective of learning different synchronization primitives, it is Ok.

[Jay]

Shouldn't we be putting the print statement also within the lock to ensure that each thread tries to print a unique value? Here the increment is mutually exclusive, but printing is not, so still the output will not match what the OP wants.

[Jay]

@AlexandruC., If each thread must have it's own copy, then why is synchronization required?

[Alexandru C.]

@Jay Yes, good point. I was focused on printing values in order that I forgot about that... Thanks.

[Brady]

@Jay, yet another good point, thanks :) If we do that, then I think the three lines could be performed atomically by just locking/unlocking once.