Unexpected number output

Question

I'm trying to create a program that solves the problem of dining philosophers using posix threads. However, I got stuck at the very beginning, since the output of std :: cout << id + 1 << "PHILOSOPHER: thinking" << std :: endl; ++ i; is incorrect and id takes too large values. Please point out my mistake.

pthread_mutex_t mutexSpoon[PHILOSOPHERS];

pthread_t createThread(int number){
    pthread_t id;
    int rc = pthread_create(&id, NULL, philFunc, &number);
    if(rc){
        abort();
    }
    return id;
}

void *philFunc(void *arg){
    srand(time(0));
    int id = *(int*)arg;
    int leftSpoon = (id>0) ? id-1 : PHILOSOPHERS;
    int rightSpoon = id;
    int temp;
    int i = 0;
    while(i < 10){
        usleep((200 - 50) * ( (double)rand() / RAND_MAX ) + 50);
        std::cout << id+1 << " PHILOSOPHER: thinking" << std::endl; ++i;

    }
    return nullptr;
}

main.cpp

using namespace std;
extern pthread_mutex_t mutexSpoon[PHILOSOPHERS];

int main(){
    setlocale(LC_ALL, "rus");


    for(int i = 0; i < PHILOSOPHERS; ++i)
        pthread_mutex_init(&mutexSpoon[i], NULL);


    vector<pthread_t> vecID(PHILOSOPHERS);

    for(int i = 0; i < PHILOSOPHERS; ++i)
        vecID[i] = createThread(i);

    for(int i = 0; i < PHILOSOPHERS; ++i)
        pthread_join(vecID[i], NULL);


    for(int i = 0; i < PHILOSOPHERS; ++i)
        pthread_mutex_destroy(&mutexSpoon[i]);
    return 0;
}

In `pthread_create(&id, NULL, philFunc, &number);` you are passing the address of a local variable. Most likely the thread reads the variable only after `createThread` returned, then `&number` becomes a dangling pointer. — Lukas-T, Jun 30 '21 at 06:47
You most likely do not want `srand(time(0));` in your thread function. You should generally call it once at the beginning of your program in `main`. — Retired Ninja, Jun 30 '21 at 06:47
You might want to prefer [C++ thread support library](https://en.cppreference.com/w/cpp/thread) over p-threads to be more portable. — Aconcagua, Jun 30 '21 at 06:58
Note that `rand` might not be thread safe: https://stackoverflow.com/questions/6161322/using-stdlibs-rand-from-multiple-threads — Alan Birtles, Jun 30 '21 at 07:45
you likely missed lines where you're using `mutexSpoon`, prehistoricpenguin wanted to point it out — Swift - Friday Pie, Jun 30 '21 at 08:26
Yes, I know I have not written lines of code where I am using `mutexSpoon`, however the problem was not caused by the code that is using `mutexSpoon`. — Enkidu, Jun 30 '21 at 23:44

Swift - Friday Pie · Accepted Answer · 2021-06-30T07:34:33.180

thread function uses an address for argument, which you pass as an address to a local variable of function createThread - number. The life span of argument should be not shorter than thread, so exactly same as the mutex. Using your snippets as base, I created an example which works around the issue:

#include <iostream>
#include <cstdlib>
#include <vector>
#include <pthread.h>
#include <unistd.h>

void *philFunc(void *arg);

#define PHILOSOPHERS 10

struct Philosopher {
    pthread_mutex_t mutexSpoon;
    pthread_t id;
    int no;
};

Philosopher philosophers[PHILOSOPHERS]  = {};

pthread_t createThread(int& number){
    pthread_t id;
    int rc = pthread_create(&id, NULL, philFunc, &number);
    if(rc){
        abort();
    }
    return id;
}

void *philFunc(void *arg){
    srand(time(0));
    int id = *(int*)arg;
    int leftSpoon = (id>0) ? id-1 : PHILOSOPHERS;
    int rightSpoon = id;
    int temp;
    int i = 0;
    while(i < 10){
        usleep((200 - 50) * ( (double)rand() / RAND_MAX ) + 50);
        std::cout << id+1 << " PHILOSOPHER: thinking" << std::endl; ++i;

    }
    return nullptr;
}

extern pthread_mutex_t mutexSpoon[PHILOSOPHERS];

int main(){
    setlocale(LC_ALL, "rus");

    for(int i = 0; i < PHILOSOPHERS; ++i) {
        pthread_mutex_init(&philosophers[i].mutexSpoon, NULL);
        philosophers[i].no = i;
        philosophers[i].id = createThread(philosophers[i].no);
    }
    
    for(int i = 0; i < PHILOSOPHERS; ++i)
        pthread_join(philosophers[i].id, NULL);


    for(int i = 0; i < PHILOSOPHERS; ++i)
        pthread_mutex_destroy(&philosophers[i].mutexSpoon);
    return 0;
}

As you see, there is now own structure Philosopher for each thread, storing its data as it should be. While philosophers here is an array, it can be any other container as long as its elements live long enough and aren't changing their addresses (requirement for some implementations of the pthread mutex).

Note that createThread(int& number) now takes its argument by reference, so the expression &number would get address of the actual object's location, not of local variable.

This code can be simpler, if using C++ thread support and std::future.

Watch out. The placement of `srand` can cause bugs at least two different ways. — user4581301, Jun 30 '21 at 07:36
What's the usage of `mutexSpoon` here? Seems it's not necessary. — prehistoricpenguin, Jun 30 '21 at 07:49
`std::cout << id+1 << " PHILOSOPHER: thinking" << std::endl;` this line can not enssure the two part will be printed in one line, we may get lines in mess. — prehistoricpenguin, Jun 30 '21 at 07:50
@prehistoricpenguin yeah, it relies on implementation of console output.. and if replacing `cout` to any other stream, it would either get messed up or cause UB (e.g. some stringstream implementations). I just didn't change anything else in code that wasn't related to problem above. As regarding the mutex, it's likely something that OP didn't show us, maybe it was output syncing. I left it in as a data somehow related to thread. The code is incomplete, some variable weren't used at all — Swift - Friday Pie, Jun 30 '21 at 08:23

Unexpected number output

1 Answers1