1

Is there any potential problem in this code snippet?

#include <mutex>
#include <map>
#include <vector>
#include <thread>

constexpr int FOO_NUM = 5;

int main()
{
std::map<int, std::mutex> mp;

std::vector<std::thread> vec;
for(int i=0; i<2; i++)
{
    vec.push_back(std::thread([&mp](){
    std::lock_guard<std::mutex> lk(mp[FOO_NUM]); //I think there is some potential problem here, am I right?
    //do something
     }));
}

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

As per the document,which says that:

Inserts value_type(key, T()) if the key does not exist. This function is equivalent to return insert(std::make_pair(key, T())).first->second;

I think there is a potential problem in the aforementioned code snippet. You see this may happen:

1.the first thread created a mutex, and is locking the mutex.

2.the second thread created a new one, and the mutex created in the first thread needs to be destroyed while it's still used in the first thread.

John
  • 2,963
  • 11
  • 33
  • Note: `-fsanitize=thread` (thread sanitizer) can help you to detect most race conditions: [godbolt example](https://godbolt.org/z/Edooxndcr). As a general rule of thumb: Reading data from multiple threads is generally fine, but if any operation would result in a modification you'll have to synchronize all threads involved (also the readers). `operator[]` of `std::map` is only threadsafe if the element already exists [see here](https://www.cplusplus.com/reference/map/map/operator[]/)) - your code would be fine if you create the mutex before the threads [example](https://godbolt.org/z/MPzxozY8n) – Turtlefight Mar 31 '22 at 02:06

2 Answers2

3

operator[] of associative and unordered containers is not specified to be safe from data races if called without synchronization in multiple threads. See [container.requirements.dataraces]/1.

Therefore your code has a data race and consequently undefined behavior. Whether or not a new mutex is created doesn't matter either.

user17732522
  • 53,019
  • 2
  • 56
  • 105
3

Yes, there is a data race, but it is even more fundamental.

None of the containers in the C++ library are thread-safe, in any way. None of their operators are thread safe.

mp[FOO_NUM]

In the shown code multiple execution threads invoke map's [] operator. This is not thread-safe, the operator itself. What's contained in the map is immaterial.

the second thread created a new one, and the mutex created in the first thread needs to be destroyed while it's still used in the first thread.

The only thing that destroys any mutex in the shown code is the map's destructor when the map itself gets destroyed when returning from main().

std::lock_guard<std::mutex> does not destroy its mutex, when the std::lock_guard gets destroyed and releases the mutex, of course. An execution thread's invocation of the map's [] operator may default-construct a new std::mutex, but there's nothing that would destroy it when the execution thread gets joined. A default-constructed value in a std::map, by its [] operator, gets destroyed only when something explicitly destroys it.

And it's the [] operator itself that's not thread safe, it has nothing to do with a mutex's construction or destruction.

Sam Varshavchik
  • 114,536
  • 5
  • 94
  • 148
  • **MAYBE**: Two threads may treat the `std::map mp;` as empty one at first. And they both try to construct a `mutex` one by one, so when the second thread is inserting `std::pair` into `mp` the `mutex` already created by the first thread needs to be destroyed.So I don't agree with you about when the `mutex` is destoryed. – John Mar 31 '22 at 01:58
  • Nope, this is already undefined behavior at this point, since the `[]` operator does not return until the new value gets default-constructed (if the specified key did not exist prior to the invocation of the `[]` operator). So this requires concurrent invocation of the `[]` operator from multiple execution threads. Undefined behavior. And if one execution thread manages to default-construct and return from the `[]` operator, the second execution thread does not need to create (or destroy) anything. – Sam Varshavchik Mar 31 '22 at 02:03
  • The const member function of the containers are thread save. – gerum Mar 31 '22 at 07:35