Pthread_join() returns incorrect values in C

Question

I met a problem when playing with the Pthreads when I test the pthread_join() between the child threads. I tried to avoid the shared variables, so I use another thread_create() function to perform the real pthread_create().

In order to track the threads, I create a pthread_t array tid[N] to store the threads and an index is also attached to each thread based on its index in the array.

However, the output shows that the some thread has the same index. I guess the variable index must be shared somehow. The index is transfered as a actual parameter into the thread_create() and the struct variable input is supposed to be cleared after the thread_create() function completes. I don't quite understand why the index becomes shared between the child threads. Is there anyone could explain this? Thanks in advance!!

To summarize,

Question 1: why the return value are not correct?

Question 2: How does the main threads wait for the other threads to complete?(I am not sure, so I use the while(1) to make sure all the threads completes.)

One note: if we uncomment the sleep() in line 38, the programs give the correct outputs.

The followings are the codes:

  1 #include <stdio.h>
  2 #include <stdlib.h>
  3 #include <pthread.h>
  4 #include <syscall.h>
  5 
  6 #define N 4
  7 
  8 typedef struct{
  9 
 10         int index;
 11         pthread_t *tid;
 12 
 13 }mix;
 14 
 15 void* thread_talk(mix* input)
 16 { 
 17         int* ret;
 18         int index = input->index;
 19         printf("In process %d, thread %u running, my index is %d\n",
 20               getpid(), pthread_self(), index);
 21               
 22         /*thread 0 won't join*/
 23         if(input->index == 0){
 24                 printf("thread 0 will stop\n");
 25                 return (void*)0;
 26         }
 27         /*Join the thread based on the index of tid*/
 28         pthread_join(input->tid[index - 1], &ret);
 29         printf("My index is %d, I receive the return value %d\n",index, ret);
 30         return (void*)(input->index);
 31 }
 32 
 33 int thread_create(pthread_t* tid, int index)
 34 {
 35         mix input = {index, tid};
 36         printf("my index is %d\n");
 37         pthread_create(&tid[index], NULL, thread_talk, (void*)(&input));
 38         //sleep(1);
 39         return 0;
 40 }
 41 
 42 int main()
 43 {
 44 
 45         pthread_t       tid[N];
 46         int             i;
 47         for(i = 0; i < N; i++)
 48                 thread_create(tid,i);
 49         while(1);       
 50         return 0;
 51 }

Answer 1

I tried to avoid the shared variables, so I use another thread_create() function to perform the real pthread_create().

... and you thereby seem to have created an even worse problem for yourself. At least, I interpret your intent to avoid shared variables to be realized by passing each thread a pointer to thread_create()'s local variable input, but, as has been observed in comments, that pointer becomes invalid as soon as thread_create() returns, immediately after starting the new thread. When the threads thereafter dereference those pointers -- which they do several times -- they produce undefined behavior.

Question 1: why the return value are not correct?

The program's behavior is undefined, therefore the return values may not reliably be what you want or expect, but there is no basis for judging whether they are correct.

Question 2: How does the main threads wait for the other threads to complete?(I am not sure, so I use the while(1) to make sure all the threads completes.)

The way for one thread to wait for another to complete is to call pthread_join(). Since each of your threads joins the previous one (or at least attempts to do so), you only need the main thread to join the last one. In fact, you mustn't allow two different threads to simultaneously attempt to join the same thread, so the last thread is the only one the main thread should attempt to join.

---

Fixing the undefinedness:

1. Get rid of thread_create(). It only confuses the matter.

2. You have two choices:

   1. Create multiple mix objects in the main thread -- e.g. an array of them -- and hand off a different one to each thread.
   2. Use one mix object in the main thread, pass it to each child, and have the child threads make internal copies. This alternative requires synchronization between the main and child threads, so that the main thread does not proceed until the new child has made its copy.

I'd go with the array, as it's easier to implement correctly:

// ...

int main() {
    pthread_t       tid[N];
    mix             input[N];
    int             i;

    for (i = 0; i < N; i++) {
        input[i].index = i;
        input[i].tid = tid;
        pthread_create(&tid[index], NULL, thread_talk, (void*)(input + i));
    }

    pthread_join(tid[N - 1], NULL);

    return 0;
}