Pointer to args struct changing too quickly, so thread does not receive correct arguments. How do I-CodePudding

Basically the parameter of my args struct is changing before the first thread has an opportunity to access the struct and to read the information it receives. So for the code below, the first thread still reads args.length as 10 instead of as 5 like its meant to. I know this because when I add sleep(1) after the first thread_create().

    pthread_t thread1, thread2;
    struct _args args;
    void *args_ptr = &args;

    args.length = 5;
    pthread_create(&thread1, NULL, array_add, args_ptr);
    args.length = 10;
    pthread_create(&thread2, NULL, array_add, args_ptr);

Can I Use a mutex to fix this?

In my program I am actually creating the threads in a for loop. So, both threads create calls, need to receive the same args_ptr; I could try to create a new args struct for every thread_create call, but that feels strange. Any advice?

CodePudding user response：

Basically the parameter of my args struct is changing before the first thread has an opportunity to access the struct and to read the information it receives.

[...]

Can I Use a mutex to fix this?

Not directly or easily. You could use the Swiss army knife of thread interaction management, a condition variable, but for a one-time pause to allow each new thread to reach a certain point before the main thread continues, I would instead use a semaphore. More on that in a bit.

First, however, you also say.

In my program I am actually creating the threads in a for loop. So, both threads create calls, need to receive the same args_ptr;

You need multiple threads to use pointers to the same struct _args object only if the threads are meant to share data with each other via that object. The way you name it and use it suggests that instead, you simply want to convey (possibly different) one-time information from the initial thread to each new thread. You do not need the same struct _args for that. You could instead create an array of them, in a manner and location that ensures that it outlives all the threads, and give each thread its own. Alternatively, you could dynamically allocate a new one for each thread, so as, again, to give each thread its own.

I could try to create a new args struct for every thread_create call, but that feels strange.

Why? If you are giving logically separate data to each thread, then why is it odd to convey it via different objects? Note well that that is a pretty good analog of standard C pass-by-value semantics.

But supposing that you really did want to use the same args object for every thread, without the main thread proceeding before each one has read it, the semaphore-based solution I mentioned would look like this:

// ...

sem_t thread_start_sem;

#define ABORT_IF_NZ(x, msg) do { \
    if ((x) != 0) {              \
        perror(msg);             \
        abort();                 \
    }                            \
} while (0)

void *thread_func(void *args) {
    // make a local copy of the argument data
    struct _args my_args = *(struct _args *) args;

    // Allow the main thread to proceed
    ABORT_IF_NZ(sem_post(&thread_start_sem), "sem_post");

    // ... args is never accessed again ...
}

int main() {
    ABORT_IF_NZ(sem_init(&thread_start_sem, 0, 0), "sem_init");

    pthread_t thread1, thread2;
    struct _args args;

    args.length = 5;
    errno = pthread_create(&thread1, NULL, thread_func, &args);
    ABORT_IF_NZ(errno, "pthread_create");
    ABORT_IF_NZ(sem_wait(&thread_start_sem), "sem_wait");

    args.length = 10;
    errno = pthread_create(&thread2, NULL, thread_func, &args);
    ABORT_IF_NZ(errno, "pthread_create");
    ABORT_IF_NZ(sem_wait(&thread_start_sem), "sem_wait");

    // ...
}

CodePudding user response：

You should not pass pointers to local objects to another thread: the object might indeed change or be discarded before the other thread gets a chance to access it.

You should instead use a separate _args structure with an appropriate lifetime for each new thread. Either a dedicated static object or one you allocate and pass a pointer to it for the thread to read its arguments from and free when no longer needed:

pthread_t thread1, thread2;
static struct _args args1, args2;

args1.length = 5;
pthread_create(&thread1, NULL, array_add, &args1);

args2.length = 10;
pthread_create(&thread2, NULL, array_add, &args2);

pthread_t thread1, thread2;
struct _args *args;

args = calloc(1, sizeof(*args));
args->length = 5;
pthread_create(&thread1, NULL, array_add, (void *)args);

args = calloc(1, sizeof(*args));
args->length = 10;
pthread_create(&thread2, NULL, array_add, (void *)args);

Note however that the array_add threads should not access the same array concurrently without a proper locking mechanism. The behavior would otherwise be undefined.

If the only argument you pass via this _args structure is an integer length, you could just cast this value as a (void *)(uintptr_t) and cast back as (int)(uintptr_t) in the thread function:

#include <stdint.h>
#include <pthread.h>

void *array_add(void *arg) {
    int length = (int)(uintptr_t)arg;
    [...]
}

int main() {
    [...]
    pthread_t thread1, thread2;

    pthread_create(&thread1, NULL, array_add, (void *)(uintptr_t)5);
    pthread_create(&thread2, NULL, array_add, (void *)(uintptr_t)10);

    [...]
}