c++thread-safetystdatomicreadwritelock

Is writing std::atomic thread safe in the area of read lock


I've a std::map<std::string, std::tuple<int, int, std::atomic<int>>> mp. The map can be read and write parallel so I use a read-write lock to keep it thread safe.

I want to know if I can write std::get<2>(mp["someKey"]) in the read lock area.

void read() {
    // read lock area
    std::shared_lock<std::shared_mutex> read_lock{mtx};
    // "someKey" exists already
    std::get<2>(mp["someKey"]).store(2);
}

I've tried to do some tests, it seems that it works but I don't know if it's incorrect and gives me a coredump someday.


Solution

  • Firstly, note that mp["someKey"] modifies the map and is not thread-safe. If no key exists yet, operator[] will create a new value-initialized key/value pair. See also Why is std::map::operator[] considered bad practice?

    Instead, you should use mp.at("someKey") or mp.find("someKey"):

    // OK, exception thrown if key doesn't exist yet
    std::get<2>(mp.at("someKey")).store(2);
    
    // OK, nothing happens if key doesn't exist yet
    if (auto it = mp.find("someKey"); it != mp.end()) {
        std::get<2>(*it).store(2);
    }
    

    .store(2); is thread-safe because writing to a std::atomic from multiple threads simultaneously is safe. The std::shared_lock isn't necessary to ensure this. However, the std::shared_lock ensures that the surrounding std::pair cannot be destroyed by another thread so it must be kept.

    Note on const-correctness

    Const-correctness would have prevented the bug with mp["someKey"]. If you only hold a read-lock, it's good practice to work with a const& to prevent accidental modifications:

    std::as_const(mp)["someKey"]); // error
    
    const auto& cmp = mp;
    cmp["someKey"]; // error
    

    Note on std::tuple

    std::tuple outside of variadic templates is always questionable. It's almost always better to have meaningful names:

    struct entry {
        int a, b; // obviously, use better names than a, b, c
        std::atomic<int> c;
    };
    
    // ...
    
    if (auto it = mp.find("someKey"); it != mp.end()) {
        it->c.store(2);
    }