pthreads not working correctly

Question

I have the following code which was simplified to show only the relevant part.

My problem is that on one machine it shows correctly the thread number and all the other values but when I run it on other machines it shows the same values for all the threads created.

I compile it with -lpthread, I even tried to compile it statically but same results.

Why does it work correctly on one machine and on others not? Is it some coding mistake or do I have to change libraries upon compilation? I am stuck. Thanks!

    pthread_mutex_t word_list;
    struct words_list {
        char myword[20];
        struct words_list * next;
    };
    struct arg_struct {
        char *myword;
        int t;
    };
    char myword[20];
    struct words_list * first_word = NULL;
            //the loading data into struct code is missing from here
    struct words_list * curr_word = first_word;
    pthread_mutex_init(&word_list,NULL);        
    int ex = 0;
    while(curr_word != NULL)
    {
        struct arg_struct args;
        int ret = -1;
        for(i = 0 ; i < max_thread; i++)
        {
            pthread_mutex_lock(&word_list);
            strncpy(myword,curr_word->myword,sizeof(myword) - 1);
            pthread_mutex_unlock(&word_list);
            args.myword = myword;
            args.t = i;

            //start threads
            if(pthread_create(&thread_id[i],NULL,&do_repl,&args) != 0)
            {
                i--;
                fprintf(stderr,RED "\nError in creating thread\n" NONE);
            }
            else
            {
                pthread_mutex_lock(&word_list);
                if(curr_word->next == NULL)
                    ex = 1;
                else
                    curr_word = curr_word->next;
                pthread_mutex_unlock(&word_list);
            }
        }//end threads creating

        for(i = 0 ; i < max_thread; i++)
        {
            void *join_result;
            if(pthread_join(thread_id[i],&join_result) != 0)
                fprintf(stderr,RED "\nError in joining thread\n" NONE);
            else
            {
                ret = *(int *)join_result;
                free(join_result);
                if((ret == 1)
                {
                    ex = 1;
                    break;
                }
            }
        }//end threads joining
        if (ex == 1)
            break;
    }//end while






    void* do_repl(void *arguments)
    {
        int *res = malloc(sizeof(int));
        struct arg_struct *args = arguments;
        char *word = args->word;
        int t = args->t;
        fprintf(stderr,"(%d) word: %s\n",t,word);
        *res = 0;
        return res;
    }

Answer 1

Your have multiple issues in this code, starting with your arg structure. The same logical arg structure is being shared by all your threads, introducing among other things, a severe race-condition:

struct arg_struct args; // << == Note. Local variable.
int ret = -1;
for(i = 0 ; i < max_thread; i++)
{
    pthread_mutex_lock(&word_list);
    strncpy(myword,curr_word->myword,sizeof(myword) - 1);
    pthread_mutex_unlock(&word_list);
    args.myword = myword;
    args.t = i;

    //start threads NOTE: args passed by address.
    if(pthread_create(&thread_id[i],NULL,&do_repl,&args) != 0)
    {
        i--;
        fprintf(stderr,RED "\nError in creating thread\n" NONE);
    }

    // rest of code...
}

Now think for a moment what happens when that thread is launched. if your for-loop launches multiple threads before some of them have a chance to access that arg-stuct and pull out their specific thread-info, then those threads will be accessing the last data your saved in it, which would be the last iteration of the loop (if you're really unlucky, you may catch it mid-update, but I'm not diving into that level of detail; the point should be obvious).

I suggest you dynamically allocate the thread arguments structure, and have the thread destroy it when finished. As a highly advised alternative (and commonly done) use that as your return value through pthread_join and then let main() destroy it after extracting the thread-finish data.

Secondly, your args struct is using a pointer for myword which is set to the same buffer for every thread (the char myword[20] local variable). Therefore, even if you "fix" your argument structure to be dynamic you still have all your threads using the same buffer.

Solution

Dynamically allocate each threads argument structure. Within that, have a local copy of the word being processed. Likewise, store the return code for the thread the args will be passed to there as well (saves you the trouble of having to allocate one in the thread and free it in main()).

// thread arguments.
struct arg_struct
{
    char myword[20];
    int ret;
    int t;
};

In your thread startup loop:

while(curr_word != NULL)
{
    int ret = -1;
    for(i = 0 ; i < max_thread; i++)
    {
        // allocate a new  argument struct for the new thread
        struct arg_struct *args = calloc(1, sizeof(*args));
        args->t = i;

        // this lock is pointless, btw.
        pthread_mutex_lock(&word_list);
        strcpy(args->myword, cur_word->myword); //note: assumes no overrun.
        pthread_mutex_unlock(&word_list);

        //start threads
        if(pthread_create(&thread_id[i],NULL, &do_repl, args) != 0)
        {
            i--;
            fprintf(stderr,RED "\nError in creating thread\n" NONE);
        }
        else
        {
            pthread_mutex_lock(&word_list);
            if(curr_word->next == NULL)
                ex = 1;
            else
                curr_word = curr_word->next;
            pthread_mutex_unlock(&word_list);
        }
    }//end threads creating

    for(i = 0 ; i < max_thread; i++)
    {
        void *join_result = NULL;
        if(pthread_join(thread_id[i], &join_result) != 0)
            fprintf(stderr,RED "\nError in joining thread\n" NONE);
        else
        {
            ret = ((struct arg_struct*)join_result)->ret;
            free(join_result);
            if((ret == 1)
            {
                ex = 1;
                break;
            }
        }
    }//end threads joining

    if (ex == 1)
        break;
}//end while

And in the thread proc, simply do this:

void* do_repl(void *arguments)
{
    struct arg_struct *args = arguments;
    fprintf(stderr,"(%d) word: %s\n", args->t, args->word);
    args->ret = 0;
    return args;
}

Sorry for any typos I may have left, but I hope you get the point.

---

EDIT The OP requested a simple thread example that launches threads with custom argument blocks. The following does just that as well as exposes the actual linked list directly to the thread crew. The threads all share a common pointer (by address, so pointer to pointer) that initially points to the list head, and is protected with a mutex (which the threads also share). All threads run until they detect the list is empty, at which point they exit. This means you could load a list significantly larger list than your pool (I chose a pool of 5, with a list of 20, but you can have many more entries in your list than that).

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


typedef struct node
{
    char myword[20];
    struct node *next;
} node;

// thread arguments.
typedef struct arg_struct
{
    pthread_mutex_t *mtx;
    node **pp;
    int ret;
    int t;
} arg_struct;


// thread procedure. doesn't do much
void* do_repl(void *arguments)
{
    arg_struct *args = arguments;

    while (1)
    {
        // lock, get, and unlock
        pthread_mutex_lock(args->mtx);
        node *p = *args->pp;
        if (p)
        {
            *args->pp = p->next;
            pthread_mutex_unlock(args->mtx);

            // print the node we just got from the list.
            fprintf(stderr,"(%d) word: %s\n", args->t, p->myword);
        }
        else
        {
            // no more entries in list. break
            break;
        }
    };

    // make sure this is released
    pthread_mutex_unlock(args->mtx);

    args->ret = 0;
    return args;
}

// main entrypoint.
int main()
{
    // very simple. we use a fixed number of threads and list nodes.
    static const int n_threads = 5;

    // build a simple forward-only linked list. will have 4x the
    //  number of threads in our crew.
    node *list = NULL;
    node **next = &list;
    int i = 0;
    for (i=0;i<n_threads*4;++i)
    {
        node *p = malloc(sizeof(*p));
        sprintf(p->myword, "String-%d", i+1);
        *next = p;
        next = &(p->next);
    }
    *next = NULL;


    // declare a mutex and thread pool for hitting all the elements
    //  in the linked list.
    pthread_mutex_t mtx = PTHREAD_MUTEX_INITIALIZER;
    pthread_t threads[n_threads];

    // lock the mutex before creating the thread pool.
    pthread_mutex_lock(&mtx);

    i = 0;
    node *shared = list;
    for (int i=0; i<n_threads; ++i)
    {
        // setup some thread arguments.
        arg_struct *args = calloc(1, sizeof(*args));
        args->mtx = &mtx;
        args->pp = &shared;
        args->t = i+1;

        // launch the thread.
        pthread_create(threads + i, NULL, do_repl, args);
    }

    // now unlatch the mutex and wait for the threads to finish.
    pthread_mutex_unlock(&mtx);
    for (i=0;i<n_threads;++i)
    {
        void *pv = NULL;
        pthread_join(threads[i], &pv);

        arg_struct *args = pv;
        fprintf(stderr,"Thread %d finished\n", args->t);
        free(args);
    }

    // cleanup the linked list.
    while (list != NULL)
    {
        node *p = list;
        list = list->next;
        free(p);
    }

    return 0;
}

Output (varies by system and run-instance)

(2) word: String-2
(1) word: String-1
(3) word: String-3
(4) word: String-4
(5) word: String-5
(2) word: String-6
(1) word: String-7
(3) word: String-8
(4) word: String-9
(5) word: String-10
(2) word: String-11
(1) word: String-12
(3) word: String-13
(4) word: String-14
(2) word: String-16
(1) word: String-17
(5) word: String-15
(3) word: String-18
(4) word: String-19
(2) word: String-20
Thread 1 finished
Thread 2 finished
Thread 3 finished
Thread 4 finished
Thread 5 finished

Note the thread id reporting each string. This proves each thread is consuming multiple entries on the list, but each is going to only one thread. The shared pointer in the arguments block ensures this (as well as the obvious mutex protection).