Issue with Threading in C for Linux

Question

Here is what I need to do:

Write a pthread program that takes an integer command line argument n, spawns n threads that will each generate a random numbers between -100 and 100, and then computes and prints out the sum of these random numbers. Each thread needs to print out the random number it generates.

Here is what I have:

#include <stdio.h>
#include <stdlib.h>
#include <pthread.h>
#include <getopt.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <time.h>
int randomNum=0;
int randomSum=0;
void *randomNumberGenerator(void *id){
    int *myid = (int *)id;
    randomNum = rand()% 201 + (-100);
    printf("%d\n", randomNum);
    randomSum+=randomNum;

}

int main (int argc , char *argv[]){
    int command;
    char *strNumThreads = NULL;
    int i;
    while((command = getopt(argc, argv, "n:"))!=-1){
        if(command == 'n'){
            strNumThreads = optarg;
            break;
        }
    }

    int numThreads = atoi(strNumThreads);
    pthread_t thread;
    int newThread;

    for(i = 0; i<numThreads; i++){
        srand(time(NULL));
        pthread_create(&thread, NULL, randomNumberGenerator, (void*)i); 

    }
    pthread_exit(NULL);
    printf("%d\n" , randomSum);

    return 0;
}

For some reason randomSum is not getting printed.

You have a data race on those variables, they should be mutex'd — Bernardo Meurer, Mar 05 '18 at 01:02
So this is the thread solution to this https://stackoverflow.com/questions/49098921/processess-and-infinite-loops-in-c-for-linux. `randomSum` and `randomNum` are shared, you need a mutex for writing in `randomSum`. `randomNum` doesn't need to be a global variable — Pablo, Mar 05 '18 at 01:18

Pablo · Accepted Answer · 2018-03-05T01:42:20.247

randomNum is a variable that is shared among all threads, so you need a mutex when you access the variable, because randomSum+=randomNum; is not an atomic operation. The current process might get interrupted and another process is scheduled which changes both variables. When the interrupted process resumes, it will overwrite randomNum and you end up with garbage.

Also you have to wait for all threads to finish until you print the sum. For that you have to execute pthread_wait.

#include <stdio.h>
#include <stdlib.h>
#include <pthread.h>
#include <getopt.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <time.h>

// can be a global variable
int randomSum=0;

pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;

void *randomNumberGenerator(void *id){
    int randomNum=0; // does not need to be a global variable
    randomNum = rand()% 201 + (-100);
    printf("%d\n", randomNum);
    pthread_mutex_lock(&mutex);
    randomSum+=randomNum;
    pthread_mutex_unlock(&mutex);

    pthread_exit(0);
}

int main (int argc , char *argv[]){
    int command;
    char *strNumThreads = NULL;
    int i;
    while((command = getopt(argc, argv, "n:"))!=-1){
        if(command == 'n'){
            strNumThreads = optarg;
            break;
        }
    }

    // initializing the randomizer
    srand(time(NULL));

    int numThreads = atoi(strNumThreads);
    if(numThreads == 0)
    {
        fprintf(stderr, "Invalid number of threads\n");
        return 1;
    }

    pthread_t threads[numThreads];

    for(i = 0; i<numThreads; i++){
        pthread_create(threads + i, NULL, randomNumberGenerator, NULL); 
    }

    for(i = 0; i < numThreads; ++i)
        pthread_join(threads[i], NULL);


    printf("%d\n" , randomSum);

    return 0;
}

You really need to learn how to use the libraries you are using. pthread_exit must be used by the threads to tell the system "I'm finished", calling it in the main thread makes no sense.

pthread_create(&thread, NULL, randomNumberGenerator, (void*)i);

I consider this an uggly hack, what you should do is create an array with the ids of the threads and pass every thread a pointer to its id, like this:

int ids[numThreads];

for(i = 0; i<numThreads; i++){
    ids[i] = i;
    pthread_create(&thread, NULL, randomNumberGenerator, ids+i);
}

and in the thread you can do

void *randomNumberGenerator(void *idp) {

    int *id = idp;

    printf("My thread id is %d\n", *id);

    ...

    pthread_exit(NULL);
}

And if your worker threads are just calculating a value, you can use pthread_exit to return that value back to the main thread. For example:

#include <stdio.h>
#include <stdlib.h>
#include <pthread.h>
#include <getopt.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <time.h>

struct thdata {
    int id;
    int random;
};


void *randomNumberGenerator(void *data) {
    struct thdata *ret = data;

    ret->random = rand()% 201 + (-100);
    printf("thread with id %d: random %d\n", ret->id, ret->random);

    pthread_exit(data);
}

int main (int argc , char *argv[]){
    int i;

    // initializing the randomizer
    srand(time(NULL));

    int numThreads = 5;
    if(numThreads == 0)
    {
        fprintf(stderr, "Invalid number of threads\n");
        return 1;
    }

    pthread_t threads[numThreads];
    struct thdata data[numThreads];

    for(i = 0; i<numThreads; i++){
        data[i].id = i;
        pthread_create(threads + i, NULL, randomNumberGenerator, data+i); 
    }

    int randomSum = 0;
    for(i = 0; i < numThreads; ++i)
    {
        struct thdata *data;
        pthread_join(threads[i], (void**) &data);
        randomSum += data->random;
    }


    printf("The sum of the random values is: %d\n" , randomSum);

    return 0;
}

Which gives me the output (for 5 threads):

thread with id 0: random 72
thread with id 4: random -94
thread with id 1: random 1
thread with id 2: random -74
thread with id 3: random 42
The sum of the random values is: -53

@BernardoMeurer I used the OPs code and fixed the places where it was wrong. — Pablo, Mar 05 '18 at 01:29

score 1 · Answer 2 · answered Mar 05 '18 at 01:27

You currently have a data race in place, because you have multiple threads accessing randomSum concurrently. Here's a solution, with comments, using Mutexes to solve the problem.

Note how using a struct to hold the sum and it's mutex allows us to get rid of all globals.

As a plus, I replaced your random generator with a proper one on POSIX systems. Note that your multiple calls to srand() are wrong, and cause less randomicity. You should only ever call srand() once, to generate the seed.

#include <stdio.h>
#include <stdbool.h>
#include <stdlib.h>
#include <limits.h>
#include <time.h>
#include <pthread.h>

static bool HAS_URANDOM = true; // Global

unsigned int random_uint() {
    unsigned int r_uint;
    // Try to open the random generator device
    FILE *f = fopen("/dev/urandom", "r");
    if (f == NULL) {
        if (HAS_URANDOM) {
            // Warn that urandom isn't working, but fallthrough to rand()
            printf("---- Failed loading random generator device /dev/urandom. Defaulting to rand().\n");
            srand((unsigned int) time(NULL));
            HAS_URANDOM = false;
        }
        r_uint = (unsigned int) rand();
    } else {
        // If we have urandom, just read from it and cast to uint
        fread(&r_uint, sizeof(r_uint), 1, f);
        fclose(f);
    }
    return r_uint;
}

// Inclusive range
// https://stackoverflow.com/a/17554531/2080712
unsigned int generate_uint(unsigned int lower, unsigned int upper) {
    if (upper - lower == UINT_MAX) {
        fprintf(stderr, "Invalid bounds on generate_int().\n");
        exit(EXIT_FAILURE);
    }
    unsigned int r_uint;
    const unsigned int range = 1 + (upper - lower);
    if (range == 0) {
        fprintf(stderr, "Invalid range!\n---- upper=%d\n---- lower=%d\n---- range=%d\n", upper, lower, range);
        exit(EXIT_FAILURE);
    }
    const unsigned int buckets = UINT_MAX / range;
    const unsigned int limit = buckets * range;
    /* Create equal size buckets all in a row, then fire randomly towards
     * the buckets until you land in one of them. All buckets are equally
     * likely. If you land off the end of the line of buckets, try again. */
    do {
        r_uint = random_uint();
    } while (r_uint >= limit);
    unsigned int res = lower + (r_uint / buckets);
    return res;
}

typedef struct {
    pthread_mutex_t lock; // Our lock to avoid data races
    long sum; // The sum value
} sum_t;

// Thread function
void *do_sum(void *arg) {
    sum_t *sum = (sum_t*)(arg); // Reinterpret the argument as sum_t
    int val = generate_uint(0, 100) - 100; // Generate an integer in the range we want
    pthread_mutex_lock(&sum->lock); // Lock the value
    sum->sum += val; // Sum
    pthread_mutex_unlock(&sum->lock); // Unlock the value
    return NULL;
}

int main(int argc, char *argv[]) {
    // Guarantee argument
    if(argc != 2) {
        printf("Please provide a number of threads.\n");
        exit(EXIT_FAILURE);
    }
    // Get our thread count
    long count = strtol(argv[1], NULL, 10);

    // Allocate threads
    pthread_t threads[count];

    // Create & initialize sum structure
    sum_t sum;
    pthread_mutex_init(&(sum.lock), NULL);
    sum.sum = 0;

    // Run sum threads
    for (long i = 0; i < count; ++i) {
        pthread_create(&(threads[i]), NULL, do_sum, &sum);
    }

    // Wait until they have finished
    for (long i = 0; i < count; ++i) {
        pthread_join(threads[i], NULL);
    }

    // Destroy the mutex lock
    pthread_mutex_destroy(&(sum.lock));

    // Print result
    printf("%ld\n", sum.sum);
    return 0;
}

Issue with Threading in C for Linux

2 Answers2