0

I'm currently working on a project which requires the use of threads. However, before tackling the project, I want to create a simple exercise for myself to test my understanding of threads.

What I have are 2x functions; one for infinitely generating random numbers and the other for printing the output of this function.

The value of this random number will be continuously updated via a pointer.

From my understanding, I will need a mutex to prevent undefined behavior when reading and writing values to this pointer. I would also need to detach the random number generator function from the main function.

However, I'm having issues trying to build the project in Visual Studio Code which I suspecting due to a flaw in my logic.

#include <bits/stdc++.h>
#include <iostream>
#include <thread>
#include <mutex>

std::mutex global_mu;

void generateRandomNum(int min, int max, int *number)
{
    while (true) {
    global_mu.lock();
    std::random_device rd;
    std::mt19937 rng(rd());
    std::uniform_int_distribution<int> uni(min, max);
    *number = uni(rng);
    global_mu.unlock();
    }
}

int main()
{
    int *int_pointer;
    int number = 0;
    int_pointer = &number;

    std::thread t1(generateRandomNum, 0, 3000, int_pointer);
    t1.detach();

    while(true) {
        global_mu.lock();
        std::cout << int_pointer << std::endl;
        global_mu.unlock();
    }
}
Oranges
  • 1
  • 1
  • 2
    what issuesdo you have ? Are there compiler errors? – 463035818_is_not_an_ai May 12 '22 at 09:07
  • 4
    You should not include : https://stackoverflow.com/questions/31816095/why-should-i-not-include-bits-stdc-h – wohlstad May 12 '22 at 09:08
  • `detach` is very rarely a good idea. Insted you should introduce some way to make `t1`s loop stop and then `join` the thread when you are done – 463035818_is_not_an_ai May 12 '22 at 09:10
  • 1) You don't need a mutex, an atomic int would be sufficient. 2) You don't want to seed the PRNG in every iteration. 3) You don't need to protect the random number generation. The generator is local to a single thread. – Daniel Langr May 12 '22 at 09:11
  • @463035818_is_not_a_number When I try running without debugging on Visual Studio Code, it gives me an error saying "launch: program "file path" does not exist" – Oranges May 12 '22 at 09:12
  • Thanks for presenting the program so well! You might improve formatting of the function, which appears with incorrect indentation. Compile-time errors are symptomatic of language issues not logic issues. To help people help you fix your compile-time errors, you should copy them into the question as well. Could you, please? – ariels May 12 '22 at 09:12
  • @Oranges Then, there is likely some problem with your VSCode project configuration. The code compiles just fine: https://godbolt.org/z/G6aGTdqT1. – Daniel Langr May 12 '22 at 09:13
  • Possibly relevant: [Can't compile code "launch: program does not exist"](https://stackoverflow.com/q/47872250/580083) – Daniel Langr May 12 '22 at 09:19
  • @DanielLangr that's very strange. Regarding the error, I sometimes get it in other instances when compiling and running, but it's often due to syntax or missing library issues. Once I've fixed it/added the library, it runs and compiles okay. Therefore I always treated the error as such. – Oranges May 12 '22 at 09:23
  • Re, "I'm having issues trying to build the project." You will get better help understanding those "issues" if you tell people exactly what issues you are having. (e.g., do you get error messages that you don't understand when you try to build it?, then please include the text of the error message in your question.) – Solomon Slow May 12 '22 at 14:21

1 Answers1

0

This looks wrong:

std::cout << int_pointer << std::endl;

You're trying to print the value of the pointer instead of printing the value of the int variable to which it points. You either should do this:

std::cout << *int_pointer << std::endl;

or this:

std::cout << number << std::endl;

This also looks like it maybe does not do what you want:

while (true) {
    ...
    std::random_device rd;
    std::mt19937 rng(rd());
    std::uniform_int_distribution<int> uni(min, max);
    *number = uni(rng);
    ...
}

You are constructing and initializing a new random number generator for each iteration of the loop. You probably should move the PRNG out of the loop:

std::random_device rd;
std::mt19937 rng(rd());
std::uniform_int_distribution<int> uni(min, max);
while (true) {
    ...
    *number = uni(rng);
    ...
}

Finally, you probably should not ever do this:

while(true) {
    global_mu.lock();
    ...
    global_mu.unlock();
}

What's the very next thing that the thread does after it calls unlock()? The next thing it does is, it re-locks the same mutex again.

I don't want to get too technical, but the problem in this situation is that the thread that is most likely to acquire the mutex will be the one that just released it, and not the one that's been waiting for a long time. Whichever thread gets in to the mutex first, is going to starve the other thread.

The way out of the starvation problem is to only lock the mutex for the least amount of time necessary. E.g.,:

void generateRandomNum(int min, int max, int *number)
{
    std::random_device rd;
    std::mt19937 rng(rd());
    std::uniform_int_distribution<int> uni(min, max);
    while (true) {
        int temp = uni(rng);
    
        global_mu.lock();
        *number = temp;
        global_mu.unlock();
    }
}

int main()
{
    int *int_pointer;
    int number = 0;
    int_pointer = &number;

    std::thread t1(generateRandomNum, 0, 3000, int_pointer);
    t1.detach();

    while(true) {
        int temp;

        global_mu.lock();
        temp = number;
        global_mu.unlock();

        std::cout << temp << std::endl;
    }
}

If this feels like you're writing a lot of extra lines, you're right. Multi-threading is hard to get right. And, in order to get high performance from a multi-threaded program, you are going to have to write extra lines of code, and maybe even make the program do more work per CPU than a single threaded program would do.

Solomon Slow
  • 25,130
  • 5
  • 37
  • 57