In general, it's not thread-safe to access the same instance of std::map from different threads.

But is it could be thread-safe under such a condition:

1. no more element would be added to the instance of std::map after it has been initialized
2. the type of values of the std::map is std::atomic<T>

Here is the demo code:

#include<atomic>
#include<thread>
#include<map>
#include<vector>
#include<iostream>

class Demo{
public:
Demo()
{
    mp_.insert(std::make_pair(1, true));
    mp_.insert(std::make_pair(2, true));
    mp_.insert(std::make_pair(3, true));
}

int Get(const int& integer, bool& flag)
{
    const auto itr = mp_.find(integer);
    if( itr == mp_.end())
    {
        return -1;
    }
    else
    {
        flag = itr->second;
        return 0;
    }
}
int Set(const int& integer, const bool& flag)
{
    const auto itr = mp_.find(integer);
    if( itr == mp_.end())
    {
        return -1;
    }
    else
    {
        itr->second = flag;
        return 0;
    }
}

private:
std::map<int, std::atomic<bool>> mp_;
};

int main()
{
    Demo demo;

    std::vector<std::thread> vec;

    vec.push_back(std::thread([&demo](){
        while(true)
        {
            for(int i=0; i<9; i  )
            {
                bool cur_flag = false;
                if(demo.Get(i, cur_flag) == 0)
                {
                    demo.Set(i, !cur_flag);
                }
                std::this_thread::sleep_for(std::chrono::milliseconds(1000));
            }
        }
    }));

    vec.push_back(std::thread([&demo](){
        while(true)
        {
            for(int i=0; i<9; i  )
            {
                bool cur_flag = false;
                if(demo.Get(i, cur_flag)==0)
                {
                    std::cout << "(" << i << "," << cur_flag <<")" << std::endl;
                }
                std::this_thread::sleep_for(std::chrono::milliseconds(10));
            }
        }
    })
    );

    for(auto& thread:vec)
    {
        thread.join();
    }
}

What more, the compiler does not complain about anything with -fsanitize=thread option.

CodePudding user response：

Yes, this is safe.

Data races are best thought of as unsynchronized conflicting access (potential concurrent reads and writes).

std::thread construction imposes an order: the actions which preceded in code are guaranteed to come before the thread starts. So the map is completely populated before the concurrent accesses.

The library says standard types can only access the type itself, the function arguments, and the required properties of any container elements. std::map::find is non-const, but the standard requires that for the purposes of data races, it is treated as const. Operations on iterators are required to at most access (but not modify) the container. So the concurrent accesses to std::map are all non-modifying.

That leaves the load and store from the std::atomic<bool> which is race-free as well.

CodePudding user response：

This should avoid data-race UB, since you aren't mutating the std::map data structure at all after starting either thread. And given the limited ways you're modifying the atomic values of the map, that's also safe.

You didn't provide accessor functions that would allow atomic RMW, so the only thing you can do is .store(flag) and .load(flag) with the default memory_order_seq_cst. Not .exchange or .compare_exchange_weak, or ^= 1 to flip.

Your if(Get) Set isn't equivalent to a flag ^= 1; atomic flip of that flag (although that doesn't compile as efficiently as one might like: Efficient way to toggle an atomic_bool).

If another thread was also flipping the same flag, they could step on each other. e.g. 10 total flips should get it back to the original value, but with separate atomic load and store, both threads could load the same value and then store the same value, resulting in only one flip for 2 (or many more) if(Get) Set operations across multiple threads.

Of course, if you don't have multiple threads writing the same flag, it is more efficient to separately load and store like you're doing.

Especially if you avoid the default memory_order_seq_cst, e.g. provide a std::memory_order ord = std::memory_order_seq_cst optional arg to your accessor functions, like std::atomic functions do. SC stores on most ISAs are more costly. (Especially on x86, where even mo_release is free in asm, but mo_seq_cst needs a full barrier.)

(But as discussed on Efficient way to toggle an atomic_bool std::atomic<bool> has no portable way to atomically flip it other than ^= 1, there isn't a member function that can take a memory_order arg. atomic<uint8_t> may be a better choice, taking the low bit as the actual boolean value.)

Since you need to be able to return failure, perhaps return a pointer to the std::atomic<bool> objects, with NULL indicating not found. That would let the caller use any std::atomic member function. But it does make misuse possible, holding onto the reference for too long.