In my code, I want to use a std::atomic_flag to synchronize two threads. Specifically, I would like to use the new wait and notify_all features that are introduced in C 20.

In a nutshell: one thread is waiting for the flag to become ready while another thread will set the flag and issue the notification. The catch, however, is that the atomic_flag lives on the stack and will be destroyed after the notification, while the first thread may still be in the call to wait.

Basically, I have something equivalent to the following snippet:

#include <atomic>
#include <thread>

int main(int, char**)
{
    auto t = std::thread{};

    {
    auto f = std::atomic_flag{};
    t = std::thread{[&f] { f.wait(false); }};

    // Ensures that 't' is waiting on 'f' (not 100% guarantee, but you get the point)
    std::this_thread::sleep_for(std::chrono::milliseconds{50});

    f.test_and_set();
    f.notify_all();
    } // <--- 'f' is destroyed here but 't' may still be in the wait call

    t.join();
    return 0;
}

In the past, I have used boost::latch for situations like this and I know from experience that this pattern will nearly always crash or assert. However, replacing boost::latch with std::atomic_flag has not resulted in any crashes, asserts, or deadlocks.

My question: is it safe to destroy a std::atomic_flag after a call to notify_all (i.e. with waking threads potentially still in the wait method)?

CodePudding user response：

No, it is not safe.

From the standard

In the standard ([atomics.flag]), the effects of atomic_flag_wait are described as follows:

Effects: Repeatedly performs the following steps, in order:

• Evaluates flag->test(order) != old.
• If the result of that evaluation is true, returns.
• Blocks until it is unblocked by an atomic notifying operation or is unblocked spuriously.

This implies that, after unblocking, the std::atomic_flag is accessed to read the new value. And thus, it is a race with destructing the atomic flag from the other thread.

In practice

Probably, the code snippet works fine because the destructor of std::atomic_flag is trivial. So the memory is left intact on the stack and the waiting thread can still continue to use those bytes as if they were the atomic flag.

By modifying the code a bit to explicitly zero the memory where the std::atomic_flag lived, the snippet now deadlocks (at least on my system).

#include <atomic>
#include <cstddef>
#include <cstring>
#include <thread>

int main(int, char**)
{
    auto t = std::thread{};

    // Some memory to construct the std::atomic_flag in
    std::byte memory[sizeof(std::atomic_flag)];

    {
    auto f = new (reinterpret_cast<std::atomic_flag *>(&memory)) std::atomic_flag{};

    t = std::thread{[&f] { f->wait(false); }};
    std::this_thread::sleep_for(std::chrono::milliseconds{50});
    f->test_and_set();
    f->notify_all();
    
    f->~atomic_flag(); // Trivial, but it doesn't hurt

    // Set the memory where the std::atomic_flag lives to all zeroes
    std::memset(&memory, 0, sizeof(std::atomic_flag));
    }

    t.join();
    return 0;
}

Which will deadlock the waiting thread if it happens to read the atomic flag's value after the memory has been set to all zeroes (probably because it now interprets those zeroes as 'false' for the atomic flag's value).