Store threads inside of a vector in C++

Question

I'm learning parallel programming and there is an example in my lecture for it. We have an array for storing 20 fibonacci numbers and basically, for computing each of them, a thread is created. This is the code for the fibonacci function:

unsigned int fibonacci(unsigned int n) {
    if(n==0)
        return 0;

    unsigned int f0 = 0, f1 = 1, f2;
    for (auto i=1u; i < n; i++) {
        f2 = f0 + f1;
        f0 = f1;
        f1 = f2;
    }
    return f1;
}

This is the code for the main method:

int main(int argc, char **argv) {
    std::vector<std::thread> threads;
    unsigned int results[20];

    for(auto i=0u; i < 20; i++) {
        auto f = rand() % 30;
        threads.push_back(std::thread([&](){
            results[i] = fibonacci(f);
        }));
    }

    std::for_each(threads.begin(), threads.end(), std::mem_fn(&std::thread::join));

    for(auto& r: results) {
        std::cout << r << " ";
    }
    std::cout << std::endl;

    return 0;
}

The numbers are calculated, but the program terminates with status -1073741819 (0xC0000005). I followed this post, but even with applying emplace.back() there is no change. So what might be the reason for this error?

`results` is 20 long (indexed from 0 to 19), and each thread will likely be writing its answer to `results[20]` which is past the end of the array. — Eljay, Mar 20 '21 at 16:20
@Eljay I couldn't understand that. Each thread writes the result to the related position in the array starting from 0. How each thread will be writing to ```results[20]``` ? — CodiClone, Mar 20 '21 at 16:24
Do you know the difference between capturing objects, in a closure, by reference vs. by value? Do you know what each one means and how it works? By the time each threads gets around to using `i` it's likely to be formally destroyed, and practically be 20. Memory corruption, undefined behavior. Ditto for the other captured variable. — Sam Varshavchik, Mar 20 '21 at 16:26
Because `i` is incremented to 20. Since each thread has a reference to `i`, its value will probably be 20 in each thread. It is a race condition. — Eljay, Mar 20 '21 at 16:27
@SamVarshavchik I know the difference between capturing objects in a reference, pointer and value. Which code row do you mind specificly? — CodiClone, Mar 20 '21 at 16:28
I am referring explicitly to your closure. What is your proof that the closure, in the new execution thread, will refer to `i` when it is still 0. What's stopping the first loop from incrementing `i` to 1, on the first iteration, before the new script references it? To 20? — Sam Varshavchik, Mar 20 '21 at 16:32
It's best to not use `[&]` and `[=]` when learning, and instead capture each variable explicitly. In your case you probably want `[&results, i, f]`. Every time you capture by `&`, you must reason about the lifetime of the captured variable with respect to the thread running in parallel with the calling thread. — rustyx, Mar 20 '21 at 16:38
@SamVarshavchik I think, I got your point. So the problem is that ```i``` in the thread isn't referencing to ```i``` from the loop? — CodiClone, Mar 20 '21 at 16:40
Worse, the lifetime of `i` and `f` could be gone by the time they are referenced. On one platform, that could result in a `0xC0000005` error. — Eljay, Mar 20 '21 at 16:43
No the problem is exactly the opposite, it is exactly referencing it. Except that C++ gives you no guarantees whatsoever that it will be referenced before the loop increments it, or even before the loop ends. It is a completely independent execution thread. Just because the thread's code appears inside the parent thread's `for` loop means absolutely nothing. — Sam Varshavchik, Mar 20 '21 at 16:46
@rustyx I'm not arguing, but this is what is represented in the lecture, so I took it as initial point for considerations. Thank you for the suggestion! — CodiClone, Mar 20 '21 at 16:46
Just to be clear: your lecture is wrong, and dangerously so (unless it demonstrated a way how to *not* write threaded code). That's very unfortunate, because these are fundamental concepts that must be taught correctly.. — rustyx, Mar 20 '21 at 16:52
@rustyx you're right. Okay if I don't pass it as a reference, then by value as a parameter in for the thread? In case of ```i```? — CodiClone, Mar 20 '21 at 16:55
Are we all in agreements that the capture should be changed from `[&]` to `[f, i, &results]`? That's the answer. — selbie, Mar 20 '21 at 17:06
Your use of `std::mem_fn()` is wrong. You are not providing it with a `std::thread` object to bind to. There is no need to use `mem_fn()` at all, just use a lambda instead (`std::for_each(..., [](thread &t){ t.join(); });`), or use a range-for loop (`for(auto &t : threads) { t.join(); }`). — Remy Lebeau, Mar 20 '21 at 17:07
@RemyLebeau At least that part is not wrong. Why do you think it compiles, then? `for_each` provides the object. — rustyx, Mar 20 '21 at 17:19
@rustyx sorry, I was thinking of `std::bind()`. But I still stand by my suggestion to use a lambda or range-for loop instead. — Remy Lebeau, Mar 20 '21 at 17:28

score 0 · Accepted Answer · answered Mar 20 '21 at 17:24

Your issue is scope (there are other problems):

for(auto i=0u; i < 20; i++) {
    // This value is reinitialized every loop.
    // Though it is likely that every loop will use the same
    // physical memory location for f this is not guaranteed by
    // the standard (especially if the loop is unrolled)
    auto f = rand() % 30;

    // Here you are capturing i, f, results by reference.
    // Because they are captured by reference your main
    // thread and the created thread are both referencing the same object.
    // 
    // One thread is modifying i, f the other is reading these value.
    // There is no attempt to make sure that the objects are accessed
    // between memory barriers (I am not sure what the modern terms are
    // thus you don't know what values you will be getting).
    threads.push_back(std::thread([&](){
        results[i] = fibonacci(f);
    }));
}
// At this point both the variable f and the variable i are out of scope.
// technically they do not exist and thus the references captured by
// the threads point to memory locations with a non live object.
//
// These locations are more than likely being used for other variables.
//
// If one of the threads accesses these memory locations after the main
// thread reaches this point then you have undefined behavior.

There are a couple of solutions.

Pass i, f by value

If you give the thread its own copy of i and f then you will have no issues with shared accesses or accesses to objects whos lifetime has ended.

    // still need to capture results by reference.
    threads.push_back(std::thread([i, f, &results](){
        results[i] = fibonacci(f);
    }));

Keep all values of `f` and pass references to these values.

unsigned int results[20];
// have an array of f one value for each thread.
int f[20];

for(auto i=0u; i < 20; i++) {
    // Assign the random to the correct location in f.
    f[i] = rand() % 30;

    // Pass references f[i] and results[i]
    // Once the reference has been captured the value of i
    // is no longer needed.
    threads.push_back(std::thread([&f = f[i], &result = results[i]](){
                result = fibonacci(f);
                }));
}

Thank you and the rest of the guys for the answers. Now I understand what they've skipped in the lecture as very important information. — CodiClone, Mar 20 '21 at 17:35

Store threads inside of a vector in C++

1 Answers1

Pass i, f by value

Keep all values of f and pass references to these values.

Keep all values of `f` and pass references to these values.