3

I am trying to finish up a program that uses multiple threads (3) to distribute a hypothetical scholarship of $4000. Each time a thread processes, it "locks" the "critical section" and blocks the other threads from taking out their chunk from the sum. With each access, the thread is meant to take 25% of the remaining sum of "scholarship" money. The output is the amount that each thread takes as it gains access to the scholarship.

My program seems to be processing proper output so far, but when it gets to the end there seems to be a catch. Each process/thread gets to a point where it doesn't terminate or exit and the program just becomes stagnant and doesn't complete. I feel like the threads are processing but not meeting a terminating condition (scholarship is all gone). The last function totalCalc() is never reached. Does anyone see anything that I don't, which could help alleviate this problem or push the program to completion?

#include <stdio.h>
#include <pthread.h>
#include <math.h>

#define PERCENTAGE 0.25

pthread_mutex_t mutex; // protecting critical section
int scholarship = 4000,
                  total = 0;
void *A();
void *B();
void *C();
void *totalCalc();

int main(){ 

    pthread_t tid1,
              tid2,
              tid3;

    //pthread_setconcurrency(3); 

    pthread_create(&tid1, NULL, (void *(*)(void *))A, NULL );
    pthread_create(&tid2, NULL, (void *(*)(void *))B, NULL );
    pthread_create(&tid3, NULL, (void *(*)(void *))C, NULL );
    pthread_join(tid1,NULL);
    pthread_join(tid2,NULL);
    pthread_join(tid3,NULL);

    totalCalc();


    return 0;

}

void *A(){
    float result;
    while(scholarship > 0){
        sleep(2);
        pthread_mutex_lock(&mutex);
        result = scholarship * PERCENTAGE;
        result = ceil(result);
        total = total + result;
        scholarship = scholarship - result;
        if( result >= 1){
            printf("A = ");
            printf("%.2f",result);
            printf("\n");
        }
        if( scholarship < 1){
            pthread_exit(0);
            printf("Thread A exited\n");
            return;
        }
        pthread_mutex_unlock(&mutex);
    }

    pthread_exit(0);

}

void *B(){
    float result;
    while(scholarship > 0){
        sleep(1);
        pthread_mutex_lock(&mutex);
        result = scholarship * PERCENTAGE;
        result = ceil(result);
        total = total + result;
        scholarship = scholarship - result;
        if( result >= 1){
            printf("B = ");
            printf("%.2f",result);
            printf("\n");
        }
        if( scholarship < 1){
            pthread_exit(0);
            printf("Thread B exited\n");
            return;
        }           
        pthread_mutex_unlock(&mutex);
    }

    pthread_exit(0);
}

void *C(){
    float result;
    while(scholarship > 0){
        sleep(1);
        pthread_mutex_lock(&mutex);
        result = scholarship * PERCENTAGE;
        result = ceil(result);
        total = total + result;
        scholarship = scholarship - result;
        if( result >= 1){
            printf("C = ");
            printf("%.2f",result);
            printf("\n");
        }
        if( scholarship < 1){
            pthread_exit(0);
            printf("Thread C exited\n");
            return;
        }           
        pthread_mutex_unlock(&mutex);       
    }

    pthread_exit(0);
}

void *totalCalc(){
    printf("Total given out: ");
    printf("%d", total);
    printf("\n");
}

Output:

B = 1000.00
C = 750.00
A = 563.00
B = 422.00
C = 317.00
B = 237.00
C = 178.00
A = 134.00
B = 100.00
C = 75.00
B = 56.00
C = 42.00
A = 32.00
B = 24.00
C = 18.00
B = 13.00
C = 10.00
A = 8.00
B = 6.00
C = 4.00
B = 3.00
C = 2.00
A = 2.00
B = 1.00
C = 1.00
B = 1.00
C = 1.00
^C
Nathan1324
  • 158
  • 1
  • 2
  • 12
  • 2
    Initialize the mutex `pthread_mutex_t mutex;` --> `pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;` – Seek Addo Aug 01 '17 at 22:55
  • 1
    these: `void *A(); void *B(); void *C();` are not valid thread function signatures; They should be: void *A( void *pData ); void *B( void *pData ); void *C( void *pData );` – user3629249 Aug 01 '17 at 23:00
  • No warnings or errors. – Nathan1324 Aug 01 '17 at 23:00
  • 1
    Once this kind of line: `pthread_exit(0);` is executed, no code after it in that that thread will be executed, so each thread has some unreachable code. BTW: that parameter should be: `NULL`, not `0` – user3629249 Aug 01 '17 at 23:01
  • For pity's sake, write the functions so that they match what `pthread_create()` expects. They can then ignore the argument, or check that it is NULL. The casts in the `pthread_create()` calls should be making you feel sick. – Jonathan Leffler Aug 01 '17 at 23:02
  • To be honest, they do! Haha professor's recommendation to copy format of some previous code for those create functions. – Nathan1324 Aug 01 '17 at 23:03
  • When compiling, always enable all the warnings, then fix those warnings. (for `gcc`, at a minimum use: `-Wall -Wextra -pedantic` I also use: `-Wconversion, -std=gnu11` ) – user3629249 Aug 01 '17 at 23:05
  • @user3629249 Here's how I am compiling: 'gcc -o scholarship scholarship.c -lpthread -lm' so after that, I include those suggested warnings? – Nathan1324 Aug 01 '17 at 23:07
  • 2
    there is a logic problem in the posted code. the threads call `pthread_exit()` before unlocking the mutex – user3629249 Aug 01 '17 at 23:08
  • @Nathan1324 really no warnings. I don't believe so. Are you compiling with JVM!!. That return in your function A"`printf("Thread A exited\n"); return;`" alone should make the gcc compiler vomit – Seek Addo Aug 01 '17 at 23:09
  • the linker options need to be on the end of the command line. Suggest: `gcc -c -ggdb -Wall -Wextra -pedantic -Wconversion -std=gnu11 -o scholarship.o scholarship.c -I.` to compile the code, then the next command should be: `gcc -ggdb scholarship.o -o scholarship -lpthread – user3629249 Aug 01 '17 at 23:40
  • Note: nothing in the `math.h` header file is being used, so should remove the statement: `#include ` and when linking do not use the `-lm` option – user3629249 Aug 01 '17 at 23:42
  • @user3629249: the `ceil()` function is declared in `` (so that header is needed). The `sleep()` function is declared in `` (so that header should be added). – Jonathan Leffler Aug 01 '17 at 23:44
  • @JonathanLeffler, Your right, I was so enamored by all the obvious problems that (my eye) skipped right over the calls to `ceil()` – user3629249 Aug 03 '17 at 01:01

3 Answers3

5

You should not write the same function out 3 times – you can pass an argument to the thread function to give it different things to do.

  • You should initialize your mutex.
  • You should use one printf() statement instead of three in a row.
  • You should unlock the mutex before you exit the thread function.
  • You should print the status before you exit the function.
  • You should return a value from the thread function when you write return.
  • There's not much virtue in the totalCalc() function once you've collapsed it into a single printf() call.
  • The term PERCENTAGE is a misnomer; it is a fraction, not a percentage.

I chose to use return instead of calling pthread_exit(); the difference is not crucial.

First set of cleanups

Here's a revision to your code.

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

#define FRACTION 0.25

static pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;
static int scholarship = 4000;
static int total = 0;
static void *calculate(void *data);

struct Data
{
    const char *name;
    int doze;
};

int main(void)
{
    pthread_t tid1;
    pthread_t tid2;
    pthread_t tid3;
    struct Data a = { "A", 2 };
    struct Data b = { "B", 1 };
    struct Data c = { "C", 1 };

    pthread_create(&tid1, NULL, calculate, &a);
    pthread_create(&tid2, NULL, calculate, &b);
    pthread_create(&tid3, NULL, calculate, &c);
    pthread_join(tid1, NULL);
    pthread_join(tid2, NULL);
    pthread_join(tid3, NULL);

    printf("Total given out: %d\n", total);

    return 0;
}

static void *calculate(void *arg)
{
    struct Data *data = arg;
    float result;
    while (scholarship > 0)
    {
        sleep(data->doze);
        pthread_mutex_lock(&mutex);
        result = scholarship * FRACTION;
        result = ceil(result);
        total = total + result;
        scholarship = scholarship - result;
        if (result >= 1)
        {
            printf("%s = %.2f\n", data->name, result);
        }
        if (scholarship < 1)
        {
            printf("Thread %s exited\n", data->name);
            pthread_mutex_unlock(&mutex);
            return 0;
        }
        pthread_mutex_unlock(&mutex);
    }

    return 0;
}

And sample output (on a Mac running macOS Sierra 10.12.6, using GCC 7.1.0):

B = 1000.00
C = 750.00
A = 563.00
B = 422.00
C = 317.00
B = 237.00
C = 178.00
A = 134.00
B = 100.00
C = 75.00
B = 56.00
C = 42.00
A = 32.00
C = 24.00
B = 18.00
C = 13.00
B = 10.00
A = 8.00
C = 6.00
B = 4.00
B = 3.00
C = 2.00
A = 2.00
B = 1.00
C = 1.00
B = 1.00
C = 1.00
Thread C exited
Thread A exited
Thread B exited
Total given out: 4000

First phase improvement

Remember: working code can usually be improved. Here's another revision of the calculate() function which has cleaner handling of the termination conditions.

static void *calculate(void *arg)
{
    struct Data *data = arg;
    while (scholarship > 0)
    {
        sleep(data->doze);
        pthread_mutex_lock(&mutex);
        float result = ceil(scholarship * FRACTION);
        total += result;
        scholarship -= result;
        if (result >= 1)
            printf("%s = %.2f\n", data->name, result);
        pthread_mutex_unlock(&mutex);
    }
    printf("Thread %s exited\n", data->name);

    return 0;
}

It still uses mixed-mode arithmetic (floating point and integer). Further improvements would involve things like modifying the main() function to use arrays instead of separate variables for thread ID and control structures. You could then have 2-26 threads rather easily. You might also use sub-second sleeps, too. You might have different threads being differently generous with what's left of the grant money — instead of a fixed fraction, you could use different fractions in different threads.


All-singing, all-dancing version

There's one problem in both the previous versions (as pointed out by user3629249 in a comment — though I already had a preliminary version of the code with the necessary fix in it; it just wasn't on SO yet). The code in the calculate() function accesses the shared variable scholarship without holding a mutex on it. That shouldn't really be done. Here's a version that handles that. It also error checks the calls to the pthread_*() functions, reporting the error and exiting if there's a problem. That's dramatic but adequate for test code. The stderr.h header and supporting source code stderr.c can be found in https://github.com/jleffler/soq/tree/master/src/libsoq. The error handling masks the operation of the code to some extent, but it is very similar to what was shown before. The main change is that the mutex is locked before entering the loop, unlocked after exiting the loop, and unlocked before sleeping and relocked after waking.

This code also uses random fractions instead of one fixed fraction, and random sub-second sleep times, and it has five threads instead of just three. It uses arrays of control structures, initializing them as it needs to. Printing the seed (the current time) is a nicety; it would allow you to reproduce the random sequence used if the program was upgraded to process command line arguments. (There'd still be the indeterminacy from thread scheduling issues.)

Note that the single calls to printf() improve the appearance of the output compared with the triple calls in the original. The original code could (and did) interleave parts of lines from different threads. With each printf() producing an entire line, that is no longer an issue. You can look at flockfile() and its friends to see what goes on — there's a sweeping statement in the specification which covers the rest of the I/O library functions.

/* SO 4544-8840 Multithreaded C program - threads not terminating */

#include "stderr.h"     // https://github.com/jleffler/soq/tree/master/src/libsoq
#include <errno.h>
#include <math.h>
#include <pthread.h>
#include <stdarg.h>
#include <stdio.h>
#include <stdlib.h>
#include <time.h>
#include <unistd.h>

static pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;
static int scholarship = 4000;
static int total = 0;
static void *calculate(void *data);

enum { MAX_THREADS = 5 };
enum { MIN_PERCENT = 10, MAX_PERCENT = 25 };

struct Data
{
    char name[2];
    struct timespec doze;
    double fraction;
};

static inline double random_fraction(void)
{
    return (double)rand() / RAND_MAX;
}

static inline _Noreturn void err_ptherror(int rc, const char *fmt, ...)
{
    errno = rc;
    va_list args;
    va_start(args, fmt);
    err_print(ERR_SYSERR, ERR_STAT, fmt, args);
    va_end(args);
    exit(EXIT_FAILURE);
}

int main(int argc, char **argv)
{
    err_setarg0(argv[argc-argc]);
    pthread_t tids[MAX_THREADS];
    struct Data ctrl[MAX_THREADS];
    unsigned seed = time(0);
    printf("Seed: %u\n", seed);
    srand(seed);
    int rc;

    for (int i = 0; i < MAX_THREADS; i++)
    {
        ctrl[i].name[0] = 'A' + i;
        ctrl[i].name[1] = '\0';
        ctrl[i].doze.tv_sec = 0;
        ctrl[i].doze.tv_nsec = 100000000 + 250000000 * random_fraction();
        ctrl[i].fraction = (MIN_PERCENT + (MAX_PERCENT - MIN_PERCENT) * random_fraction()) / 100;
        if ((rc = pthread_create(&tids[i], NULL, calculate, &ctrl[i])) != 0)
            err_ptherror(rc, "Failed to create thread %d\n", i);
    }

    for (int i = 0; i < MAX_THREADS; i++)
    {
        if ((rc = pthread_join(tids[i], NULL)) != 0)
            err_ptherror(rc, "Failed to join thread %d\n", i);
    }

    printf("Total given out: %d\n", total);

    return 0;
}

static void *calculate(void *arg)
{
    struct Data *data = arg;
    printf("Thread %s: doze = 0.%03lds, fraction = %.3f\n",
           data->name, data->doze.tv_nsec / 1000000, data->fraction);
    int rc;
    if ((rc = pthread_mutex_lock(&mutex)) != 0)
        err_ptherror(rc, "Failed to lock mutex (1) in %s()\n", __func__);
    while (scholarship > 0)
    {
        if ((rc = pthread_mutex_unlock(&mutex)) != 0)
            err_ptherror(rc, "Failed to unlock mutex (1) in %s()\n", __func__);
        nanosleep(&data->doze, NULL);
        if ((rc = pthread_mutex_lock(&mutex)) != 0)
            err_ptherror(rc, "Failed to lock mutex (2) in %s()\n", __func__);
        double result = ceil(scholarship * data->fraction);
        total += result;
        scholarship -= result;
        if (result >= 1)
            printf("%s = %.2f\n", data->name, result);
    }
    if ((rc = pthread_mutex_unlock(&mutex)) != 0)
        err_ptherror(rc, "Failed to unlock mutex (2) in %s()\n", __func__);
    printf("Thread %s exited\n", data->name);

    return 0;
}

You could still make a case for the code to be revised so it checks the scholarship amount after sleeping, breaking an infinite loop in the body of the loop. Such changes are left as a minor exercise for the reader.

Example run

Seed: 1501727930
Thread A: doze = 0.119s, fraction = 0.146
Thread B: doze = 0.199s, fraction = 0.131
Thread C: doze = 0.252s, fraction = 0.196
Thread D: doze = 0.131s, fraction = 0.102
Thread E: doze = 0.198s, fraction = 0.221
A = 584.00
D = 349.00
E = 678.00
B = 314.00
A = 303.00
C = 348.00
D = 146.00
A = 187.00
D = 112.00
E = 217.00
B = 100.00
A = 97.00
C = 111.00
D = 47.00
E = 90.00
A = 47.00
B = 36.00
D = 24.00
A = 31.00
C = 36.00
D = 15.00
E = 29.00
B = 13.00
A = 13.00
D = 8.00
A = 10.00
E = 13.00
B = 6.00
C = 8.00
D = 3.00
A = 4.00
D = 3.00
E = 4.00
B = 2.00
A = 2.00
C = 2.00
D = 1.00
A = 2.00
E = 2.00
B = 1.00
A = 1.00
D = 1.00
Thread D exited
Thread C exited
Thread A exited
Thread E exited
Thread B exited
Total given out: 4000
Jonathan Leffler
  • 730,956
  • 141
  • 904
  • 1,278
  • That's so much more clear than what I wrote! Thanks for the help – Nathan1324 Aug 01 '17 at 23:23
  • the checking of the common variable `scholarship` to be greater than 0, when the mutex is not locked, results in a race condition. Therefore, depending on exactly the order of execution, the code could result in spending more that the total of `scholarship` – user3629249 Aug 03 '17 at 01:10
  • @user3629249: yes, you're right. I had an amended but not fully checked version of the code with that fix in place, and some other changes too. I've more fully checked the code and included it into the answer now, along with a brief discussion of why. There are various other changes in that version of the code too. – Jonathan Leffler Aug 03 '17 at 02:46
2

You should unlock mutexes before returns.

if( scholarship < 1){
    pthread_exit(0);
    printf("Thread A exited\n");
    return;
}
pthread_mutex_unlock(&mutex);

Those mutexes are never unlocked. Put a

pthread_mutex_unlock(&mutex);

before

return;
hedgar2017
  • 1,425
  • 3
  • 21
  • 40
  • 2
    The mutex should be unlocked before the `pthread_exit()` call (which never returns). The `printf()` and `return` are never executed in this `if` statement. – Michael Burr Aug 01 '17 at 23:11
  • Thank you! This allowed the program to run to completion. – Nathan1324 Aug 01 '17 at 23:14
  • @hedgar2017: pthreads don't have to be terminated using `[thread_exit()` - you can simply return from the thread function. However, regardless of how you terminate the thread, it needs to clean up any necessary resources (such as mutexes that are being held). – Michael Burr Aug 02 '17 at 07:05
1

As suggested, the functions were incorrectly written in a manner that prevented the threads from exiting properly and the functions were not returning. To alleviate the program from hanging before completion I put the thread exit and return statement outside of the locked mutex, which allowed the program to execute to completion.

void *A(){
    float result;
    while(scholarship > 0){
        sleep(2);
        pthread_mutex_lock(&mutex);
        result = scholarship * PERCENTAGE;
        result = ceil(result);
        total = total + result;
        scholarship = scholarship - result;
        if( result >= 1){
            printf("A = ");
            printf("%.2f",result);
            printf("\n");
        }
        pthread_mutex_unlock(&mutex);
    }

    pthread_exit(0);
    return;

}

void *B(){
    float result;
    while(scholarship > 0){
        sleep(1);
        pthread_mutex_lock(&mutex);
        result = scholarship * PERCENTAGE;
        result = ceil(result);
        total = total + result;
        scholarship = scholarship - result;
        if( result >= 1){
            printf("B = ");
            printf("%.2f",result);
            printf("\n");
        }           
        pthread_mutex_unlock(&mutex);
    }

    pthread_exit(0);
    return;
}

void *C(){
    float result;
    while(scholarship > 0){
        sleep(1);
        pthread_mutex_lock(&mutex);
        result = scholarship * PERCENTAGE;
        result = ceil(result);
        total = total + result;
        scholarship = scholarship - result;
        if( result >= 1){
            printf("C = ");
            printf("%.2f",result);
            printf("\n");
        }           
        pthread_mutex_unlock(&mutex);       
    }

    pthread_exit(0);
    return;
}
Nathan1324
  • 158
  • 1
  • 2
  • 12
  • a good step in the right direction. In general, when programming in C, write the functions per the `man` page for the function, rather than trying to 'shortcut' those details that your code is not using. – user3629249 Aug 01 '17 at 23:45
  • these code lines ` pthread_exit(0); return;` are wrong. the `return` statement will never be executed as it is unreachable code. The call to `pthread_exit()` exits the thread, so (as indicated in a previous comment) those two lines need to be reduced to `pthread_exit( NULL );` – user3629249 Aug 01 '17 at 23:48
  • As previously indicated, the prototype for a thread function is: `void * function( void *pData )` It is best to NOT use implicit conversions nor the side effects of a function with an empty parameter list – user3629249 Aug 01 '17 at 23:50