Wrong argument cast in pthread

what I'm trying to do is send the integer value 0 to the function to use it as an index of my array. But instead of writing to patients[0], it writes to patients[1]. Any idea why? I am simple looping from 0 to 1, just to see if it's passing the value 0 correctly, passing i(0) to function, assign myArr[0] to something, but it assigns to myArr[1] instead.

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

typedef struct patient_info {
    pthread_t thread;
    char treatment;
    char department[20];
} patient;

patient patients[1000];

void* registration(void* arg)
{
    int p_num = *((int*)arg); // my array index that supposed to be 0

        if (rand() % 2 == 0)
        {
            patients[p_num].treatment = 'M';
        }
        else
        {
            patients[p_num].treatment = 'S';
        }

    return NULL;
}

int main(void)
{
    srand(time(NULL));

    for (size_t i = 0; i < 1; i  ) // simple for loop to create my thread
    {
        if (pthread_create(&patients[i].thread, NULL, &registration, (void*)&i) != 0)
        {
            perror("There has been an error with pthread_create().");
            return 1;
        }
    }

    for (size_t j = 0; j < 1; j  )
    {
        if (pthread_join(patients[j].thread, NULL) != 0)
        {
            perror("There has been an error with the pthread_join().");
            return 2;
        }
    }

    for (size_t i = 0; i < 1000; i  ) // make this loop to see where it is writing.
    {
        if (patients[i].treatment == 'M' || patients[i].treatment == 'S')
        {    
            printf("Treatment is: %c %d\n", patients[i].treatment, i);
        }        
    }
    return 0;
}

CodePudding user response：

You are passing a pointer to i, so each thread points to the same i variable.

Thus, the threads race to get their value. (e.g.) threadA wants 0 and threadB wants 1. But, if the main task is fast enough both might see either 0 or 1. Thus, a conflict.

Also, in main, i is a size_t but in registration, it's an int pointer. They are [probably] different sizes.

The solution is to pass i by value

pthread_create(&patients[i].thread, NULL, &registration, (void *) i)

And, in registration, we accept by value:

void *
registration(void *arg)
{
    size_t p_num = (size_t) arg;

    // ...

    return (void *) 0;
}

Here's the corrected code:

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

typedef struct patient_info {
    pthread_t thread;
    char treatment;
    char department[20];
} patient;

patient patients[1000];

void *
registration(void *arg)
{
    // my array index that supposed to be 0
// NOTE/BUG: this uses the wrong size pointer and to prevent the race condition
// we want to accept by value
#if 0
    int p_num = *((int *) arg);
#else
    size_t p_num = (size_t) arg;
#endif

    if (rand() % 2 == 0) {
        patients[p_num].treatment = 'M';
    }
    else {
        patients[p_num].treatment = 'S';
    }

    return NULL;
}

int
main(void)
{
    srand(time(NULL));

    // simple for loop to create my thread
    for (size_t i = 0; i < 1; i  ) {
        if (pthread_create(&patients[i].thread, NULL, &registration,
#if 0
            (void *) &i) != 0) {
#else
            (void *) i) != 0) {
#endif
            perror("There has been an error with pthread_create().");
            return 1;
        }
    }

    for (size_t j = 0; j < 1; j  ) {
        if (pthread_join(patients[j].thread, NULL) != 0) {
            perror("There has been an error with the pthread_join().");
            return 2;
        }
    }

    // make this loop to see where it is writing.
    for (size_t i = 0; i < 1000; i  ) {
        if (patients[i].treatment == 'M' || patients[i].treatment == 'S') {
            printf("Treatment is: %c %d\n", patients[i].treatment, i);
        }
    }

    return 0;
}

Since you've gone to the trouble of creating a patient struct, we can clean up the code a bit by using and passing around some pointers to that struct:

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

typedef struct patient_info {
    pthread_t thread;
    char treatment;
    char department[20];
} patient;

patient patients[1000];

void *
registration(void *arg)
{
    patient *pt = arg;

    if (rand() % 2 == 0) {
        pt->treatment = 'M';
    }
    else {
        pt->treatment = 'S';
    }

    return NULL;
}

int
main(void)
{
    srand(time(NULL));

    patient *pt;

    // simple for loop to create my thread
    for (size_t i = 0; i < 1; i  ) {
        pt = &patients[i];

        if (pthread_create(&pt->thread, NULL, &registration, pt) != 0) {
            perror("There has been an error with pthread_create().");
            return 1;
        }
    }

    for (size_t j = 0; j < 1; j  ) {
        pt = &patients[j];

        if (pthread_join(pt->thread, NULL) != 0) {
            perror("There has been an error with the pthread_join().");
            return 2;
        }
    }

    // make this loop to see where it is writing.
    for (size_t i = 0; i < 1000; i  ) {
        pt = &patients[i];

        if (pt->treatment == 'M' || pt->treatment == 'S') {
            printf("Treatment is: %c %d\n", pt->treatment, i);
        }
    }

    return 0;
}

Note that we define the patient array to have 1000 elements.

At present, we are only creating one thread.

Presumably, we want to process all 1000 records.

But, creating 1000 threads is problematic and doesn't scale too well. If we had 100,000 patients, we [probably] could not create 100,000 threads in parallel.

And, even if we could, the system would spend most of its time switching between threads and the system would slow to a crawl.

Better to have a "pool" of "worker" threads and feed them a few records at a time.

If we do that, there's no reason to put the pthread_t into the patient record. We can have two separate arrays: one for patients and another [smaller] array for "active" threads.

There are many ways to do this. Ideally, we monitor thread completion and add new threads dynamically. But, that's a bit complicated for a first try.

Here's a version that splits things up into limited chunks. It's the "good enough for now" solution:

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

typedef struct patient_info {
    char treatment;
    char department[20];
} patient;

#define NPATIENT    1000
patient patients[NPATIENT];

#define NWORKER     10
pthread_t threads[NWORKER];

void *
registration(void *arg)
{
    patient *pt = arg;

    if (rand() % 2 == 0) {
        pt->treatment = 'M';
    }
    else {
        pt->treatment = 'S';
    }

    return NULL;
}

int
main(void)
{
    srand(time(NULL));

    patient *pt;

    for (size_t patlo = 0;  patlo < NPATIENT;  patlo  = NWORKER) {
        size_t pathi = patlo   NWORKER;

        if (pathi > NPATIENT)
            pathi = NPATIENT;

        size_t itsk;

        // simple for loop to create my thread
        itsk = 0;
        for (size_t ipat = patlo; ipat < pathi; ipat  , itsk  ) {
            pt = &patients[ipat];

            if (pthread_create(&threads[itsk], NULL, &registration, pt) != 0) {
                perror("There has been an error with pthread_create().");
                return 1;
            }
        }

        // join this chunk of threads
        itsk = 0;
        for (size_t ipat = patlo; ipat < pathi; ipat  , itsk  ) {
            pt = &patients[ipat];

            if (pthread_join(threads[itsk], NULL) != 0) {
                perror("There has been an error with the pthread_join().");
                return 2;
            }
        }
    }

    // make this loop to see where it is writing.
    for (size_t ipat = 0; ipat < NPATIENT; ipat  ) {
        pt = &patients[ipat];

        if (pt->treatment == 'M' || pt->treatment == 'S') {
            printf("Treatment is: %c %zu\n", pt->treatment, ipat);
        }
    }

    return 0;
}