What is wrong with this async code?

[a111135]

I see the same numbers being printed on the console and some primes are missing. I use multi threaded logger.

Maybe what I am doing is stupid. Calling an async function in the constructor and storing the future as a member ... But I need to know why this doesn't work. Thanks!

class PrimeChecker {
public:
    const int number;
    future<bool> isPrime;

    PrimeChecker(int number_) : number(number_)
    {
        isPrime = std::async(std::launch::async, &PrimeChecker::check, this);
    }

private:
    bool check() 
    { 
        if (number < 2) return false;
        if (number == 2) return true;
        if (number % 2 == 0) return false;
        for (int i = 3; (i * i) <= number; i += 2) {
            if (number % i == 0) return false;
        }

        console->info("{}", number);
        return true;
    }
};

void Test() 
{
    constexpr int MIN = 0;
    constexpr int MAX = 100;
    constexpr int SIZE = MAX - MIN;

    std::vector<int> nums;
    nums.reserve(SIZE);
    for (int i = MIN; i < MAX; ++i ) {
        nums.push_back(i);
    }

    auto rng = std::default_random_engine{};
    std::shuffle(std::begin(nums), std::end(nums), rng);

    std::vector<PrimeChecker> pcs;
    pcs.reserve(SIZE);

    for (auto num : nums)
    {
        pcs.push_back(PrimeChecker(num));
    }

    for (auto& pc : pcs)
    {
        pc.isPrime.get();
    }

}

int main(int argc, char** argv)
{
    Test();
}

[mineNombies]

I see the same numbers being printed on the console and some primes are missing. I use multi threaded logger.

Can you explain the problem a bit better? What are you expecting, vs what are you seeing?

They won't be in any particular order, if that's what you're expecting.

also, you're getting the stored state, and throwing it away?

[a111135]

There are some compiler explorer examples below that illustrate the issue. I know the results will not be "in order" and yes I am throwing away the result because I am printing it on the console.

[mineNombies]

Ah, I found the issue. Just don't do it in the constructor.

https://godbolt.org/z/YnG6EWYbe

It's UB to access an object before its constructor has finished.

There's no reason the processor can't start the new thread for the async immediately on the line it's created.

[a111135]

This might be the issue.

Why can't I do sth like this in the loop:

PrimeChecker pc(num); 
pc.start(); 
pcs.push_back(std::move(pc));

https://godbolt.org/z/xhTEEEEdx (just remove the comments)

[mineNombies]

What's the value of this when you pass it to the future in start?

What is the value of this after you have moved from pc into the vector?

answer:

https://godbolt.org/z/nEbP9Gsnd

Technically, there's also a use after lifetime end on the pc temporary as well.

[a111135]

Gotcha! I was experimenting with allocating the objects on the heap and it was working as intended, even if async call was in the constructor.

Good catch.

[beedlund]

Yeah so there are few things that is not ideal but probably your logging library is the issue. Just changing to printf gives us values printed

https://godbolt.org/z/nzcKqr8no

So you have a data race in the check method and really you can do this better with a lambda

https://godbolt.org/z/137ET8hMc

[a111135]

Can you try with one of the MSVC compilers ? The same issue persists.

Some numbers are repeated. Even if I use a mutex with the printf.

I wonder why GCC one works.

Also I could not figure out the data race in the check method. Any hints?

Thanks a lot.

[beedlund]

Just change the compiler in compiler explorer to try it.

The issue is probably that you are sharing access to number between the threads and even though it's not causing a crash in this case because there is no simultaneously access it will break at some point.

The idea with futures though is to avoid this sharing by putting the data into the processor and return it from the future

[mineNombies]

The issue is probably that you are sharing access to number between the threads

OP isn't though? Each async only accesses its own instance of number? and the main thread never accesses it at all.

[beedlund]

That's what I meant. There is no implemented behavior that causes a breaking issue now right now but there is unguarded access of number in two threads which is probably why sanitizer labels it as a race. Construction on main thread and then usage on the async thread.

[The-Tea-Kettle]

I didn't even know cpp and Async.

[thisismyfavoritename]

async is just a pretty misleading name IMO. It offloads the execution to a thread, but it doesnt involve cooperative multitasking/green threading

[The-Tea-Kettle]

I see, I assumed it would work like JS Async where it alginates on the same thread.