How can I pass the index of a for loop as the argument for pthread_create

Question

I am using a for loop to create a number of threads and passing the index i as an argument as follows:

pthread_t p[count];
for (int i = 0; i < count; i++){
    pthread_create(&p[i], NULL, &somefunc, (void*)&i);
}

Then I attempt to retrieve the value of i:

void *somefunc (void* ptr){
    int id = *(int*)ptr;
}

However, I noticed that sometimes, id in the threads will have overlapping values which I suspect is due to the index of the for loop updating before the thread is able to retrieve the value (since I passed in the pointer, as opposed to the value itself). Does anyone have any suggestions to overcome this issue without slowing down the for loop?

Thanks

Assemblage

Answer 1

This is happening because once you pass a pointer to i you now have multiple threads using the same value. This causes a data race because the first thread is modifying i and your second thread is expecting it to never change. You can always allocate a temporary int and pass it to the thread function.

pthread_create(&p[i], NULL, &somefunc, new int(i));

This will allocate an integer in dynamic storage (heap) and initialize it with the value of i. A pointer to the newly allocated integer will then be passed to the thread function.

Then in the thread function you can take the value passed as you already do and then delete the int object.

void *somefunc (void* ptr){
    int id = *(int*)ptr;
    delete (int*)ptr;
}

[Suggestion: Avoid C style casts.]

Brighton

Answer 2

As others have said, you're passing a pointer to an object that's being modified by another thread (the parent) and accessing it without any synchronization. This is an error.

There are at least 3 solutions:

1. Allocate (via new in C++ or malloc in C) space for a single int, and have the new thread be responsible for freeing it. This is probably the worst solution because now you have an extra failure case to handle (failure to allocate) and thus it complicates and clutters your code.

2. Cast the integer to void * and back. This will surely work on any real-world POSIX system, but it's not "guaranteed" to work, and perhaps more annoyingly, it may incur warnings. You can avoid the warnings with an intermediate cast through uintptr_t.

3. Instead of passing an index, pass an address:

   pthread_create(&p[i], NULL, &somefunc, &p[i]);

Then the start function can recover the index (if it needs it for anything) by subtracting p:

int id = (pthread_t *)ptr - p;

Eamon

+1 for enumerating options with pros/cons. 3 is clever, but functions launched may lack access to p (I'd go so far as to say they typically shouldn't know of or depend on such details of their creator). – Rhianna

I agree, but (3) does seem to apply in OP's case. By the way, if the number of indices is small, you can use the same principle but with a dummy array: static const char dummy[100]; and then passing &dummy[i] to the start function as a roundabout way to pass i. – Eamon

Answer 3

You've made this a little too complicated:

for (int i = 0; i < count; i++){
    pthread_create(&p[i], NULL, &somefunc, (void*)&i);

You just want to pass the value, not a pointer to it, so pass (void*)i. As is, you're passing each thread a pointer to i which has problems:

• i will likely have left scope by the time the thread tries to read from its address - some other variable might be there instead or the memory might be sitting around unused with who-knows-what left in it
• the next iteration in the loop will overwrite the value anyway, which means all threads would be likely to see the value "count" when they dereference the pointer, if it hadn't been clobbered as above, and except for rare cases where the launching thread is suspended during looping allowing a thread it spawned to read some earlier i value

So:

for (int i = 0; i < count; i++){
    pthread_create(&p[i], NULL, &somefunc, (void*)i);

...
void *somefunc (void* id_cast_to_voidptr){
    int id = (int)id_cast_to_voidptr;
}

Rhianna

I am getting a number of cast warnings/errors for both (void*)i and (int)id_cast_to_voidptr. – Assemblage

Use an intermediate cast though uintptr_t to get rid of the warning. – Eamon

This seems to be the same. There is no difference to initialize the variable in the for loop. This definitely works for JS but not for C – Echoechoic

@CarlosEmilianoCastroTrejo: "This seems to be the same." - what seems to be the same as what? If you mean the top snippet of code is the same as in the question - yes, deliberately, so I can have it there for easy reference while explaining why it doesn't work. " There is no difference to initialize the variable in the for loop." - correct - there was nothing wrong with the for loop, so no reason to make anything different. Nothing you say makes any sense. Of course it works for C. – Rhianna

Answer 4

I think the best answer is to declare an array of args at the beginning, with the same size as the amount of threads you intend to create.

That way those values are never overwritten or in a race condition.

int args[count];

for (int i = 0; i < count; i++){
     args[i]=i;
     pthread_create(&p[i], NULL, &somefunc, (void*)args[i]);
}

Tammy

Answer 5

You can declare an array of tasks id at the beginning of the main function. Whenever you create the thread you can send the index of that array to prevent the overtwriting of the parameter.

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

#define NUM_THREADS 8

void *PrintHello(void *threadid)
{
   long taskid;
   sleep(1);
   taskid = (long) threadid;
   printf("Hello from thread %ld\n", taskid);
   pthread_exit(NULL);
}

int main(int argc, char *argv[])
{
   pthread_t threads[NUM_THREADS];
   long taskids[NUM_THREADS];
   int rc;

   for(long t=0;t<NUM_THREADS;t++) {
      printf("Creating thread %ld\n", t);
      taskids[t] = t;
      rc = pthread_create(&threads[t], NULL, PrintHello, (void *) taskids[t]);
      
      if (rc) {
         printf("ERROR; return code from pthread_create() is %d\n", rc);
         exit(-1);
         }
   }

   pthread_exit(NULL);
}

Echoechoic

No wonder you didn't understand and downvoted my answer - you don't even understand your own. (void *) taskids[t] simply casts the value you've stored in taskids[t] to a void*, so you could have passed (void*)t and got the exact same behaviour, without having a taskids array at all. The only real difference between my answer and yours is you choose to cast to long whereas I used int, and your main function shouldn't call pthread_exit - it should join the threads it created so main() doesn't terminate before them. – Rhianna