Accessing random number engine from multiple threads

Question

this is my first question, so please forgive me any violations against your policy. I want to have one global random number engine per thread, to which purpose I've devised the following scheme: Each thread I start gets a unique index from an atomic global int. There is a static vector of random engines, whose i-th member is thought to be used by the thread with the index i. If the index if greater than the vector size elements are added to it in a synchronized manner. To prevent performance penalties, I check twice if the index is greater than the vector size: once in an unsynced manner, and once more after locking the mutex. So far so good, but the following example fails with all sorts of errors (heap corruption, malloc-errors, etc.).

#include<vector>
#include<thread>
#include<mutex>
#include<atomic>
#include<random>
#include<iostream>

using std::cout;

std::atomic_uint INDEX_GEN{};
std::vector<std::mt19937> RNDS{};
float f = 0.0f;
std::mutex m{};

class TestAThread {
public:
TestAThread() :thread(nullptr){
    cout << "Calling constructor TestAThread\n";
    thread = new std::thread(&TestAThread::run, this);
}

TestAThread(TestAThread&& source) : thread(source.thread){
    source.thread = nullptr;
    cout << "Calling move constructor TestAThread. My ptr is " << thread << ". Source ptr is" << source.thread << "\n";
}

TestAThread(const TestAThread& source) = delete;

~TestAThread() {
    cout << "Calling destructor TestAThread. Pointer is " << thread << "\n";
    if (thread != nullptr){
        cout << "Deleting thread pointer\n";
        thread->join();
        delete thread;
        thread = nullptr;
    }
}

void run(){
    int index = INDEX_GEN.fetch_add(1);
    std::uniform_real_distribution<float> uniformRnd{ 0.0f, 1.0f };

    while (true){
        if (index >= RNDS.size()){
            m.lock();
            // add randoms in a synchronized manner.
            while (index >= RNDS.size()){
                cout << "index is " << index << ", size is " << RNDS.size() << std::endl;
                RNDS.emplace_back();
            }
            m.unlock();
        }

        f += uniformRnd(RNDS[index]);
    }
}

std::thread*    thread;
};

int main(int argc, char* argv[]){
std::vector<TestAThread> threads;
for (int i = 0; i < 10; ++i){
    threads.emplace_back();
}

cout << f;
}

What am I doing wrong?!

Btw, m.lock(); RNDS.resize(index+1); m.unlock(); should work fine. — Danvil
– Danvil, Commented Apr 19, 2014 at 17:18

rici · Accepted Answer · 2014-04-19 19:26:29Z

2

Obviously f += ... would be a race-condition regardless of the right-hand side, but I suppose you already knew that.

The main problem that I see is your use of the global std::vector<std::mt19937> RNDS. Your mutex-protected critical section only encompasses adding new elements; not accessing existing elements:

... uniformRnd(RNDS[index]);

That's not thread-safe because resizing RNDS in another thread could cause RNDS[index] to be moved into a new memory location. In fact, this could happen after the reference RNDS[index] is computed but before uniformRnd gets around to using it, in which case what uniformRnd thinks is a Generator& will be a dangling pointer, possibly to a newly-created object. In any event, uniformRnd's operator() makes no guarantee about data races [Note 1], and neither does RNDS's operator[].

You could get around this problem by:

computing a reference (or pointer) to the generator within the protected section (which cannot be contingent on whether the container's size is sufficient), and
using a std::deque instead of a std::vector, which does not invalidate references when it is resized (unless the referenced object has been removed from the container by the resizing).

Something like this (focusing on the race condition; there are other things I'd probably do differently):

std::mt19937& get_generator(int index) {
    std::lock_guard<std::mutex> l(m);
    if (index <= RNDS.size()) RNDS.resize(index + 1);
    return RNDS[index];
}
void run(){
    int index = INDEX_GEN.fetch_add(1);
    auto& gen = get_generator(index);
    std::uniform_real_distribution<float> uniformRnd{ 0.0f, 1.0f };
    while (true) {
        /* Do something with uniformRnd(gen); */
    }
}

[1] The prototype for operator() of uniformRnd is template< class Generator > result_type operator()( Generator& g );. In other words, the argument must be a mutable reference, which means that it is not implicitly thread-safe; only const& arguments to standard library functions are free of data races.

answered Apr 19, 2014 at 19:26

rici

243k30 gold badges263 silver badges364 bronze badges

Sign up to request clarification or add additional context in comments.

5 Comments

Mischa Over a year ago

Thanks for your extensive reply. I'll try your code out tomorrow. In the meantime, what are those things you'd do differently? I don't really stick to my design, if you have better ones, I'd be delighted to hear it :-)

rici Over a year ago

@Mischa: I'd avoid globals, for one thing :) Also, you might be able to make use of thread-local storage. In general, codereview.stackexchange.com is better suited for code reviews.

Mischa Over a year ago

This was my first idea, too, but MSVC++ wouldn't allow anything with a non-trivial constructor to be declared thread-local. It was only reading your post, I realized that I still may declare a pointer thread-local. Thanks for the reply and inspiration. Issue definitely closed. I'd upvote your reply, but my reputation as a newb sucks :-)

rici Over a year ago

@mischa: Cool. I think you can accept the answer even as a newbie.

Mischa Over a year ago

well it seems I need at least 250 reputation points for that :-/ Anyway, there is no accept option.

Collectives™ on Stack Overflow

Accessing random number engine from multiple threads

1 Answer 1

5 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

5 Comments

Your Answer

Sign up or log in

Post as a guest

Related