I have an unordered_map and I'm using mutex locks for emplace and delete, find operations, but I don't use a mutex when modifying map's elements, because I don't see any point. but I'm curious whether I'm wrong in this case.

Should I use one when modifying element value?

std::unordred_map<std::string, Connection> connections;

// Lock at Try_Emplace
connectionsMapMutex.lock();
auto [element, inserted] = connections.try_emplace(peer);
connectionsMapMutex.unlock();


// No locks here from now
auto& connection = element->second;
// Modifying Element
connection.foo = "bar";

CodePudding user response：

Consider what can happen when you have one thread reading from the map and the other one writing to it:

1. Thread A starts executing the command string myLocalStr = element->second.foo;
2. As part of the above, the std::string copy-constructor starts executing: it stores foo's character-buffer-pointer into a register, and starts dereferencing it to copy out characters from the original string's buffer to myLocalStr's buffer.
3. Just then, thread A's quantum expires, and thread B gains control of the CPU and executes the command connection.foo = "some other string"
4. Thread B's assignment-operator causes the std::string to deallocate its character-buffer and allocate a new one to hold the new string.
5. Thread A then starts running again, and continues executing the std::string copy-constructor from step 2, but now the pointer it is dereferencing to read in characters is no longer pointing at valid data, because Thread A deleted the buffer! Poof, Undefined Behavior is invoked, resulting in a crash (if you're lucky) or insidious data corruption (if you're unlucky, in which case you'll be spending several weeks trying to figure out why your program's data gets randomly corrupted only about once a month).

And note that the above scenario is just on a single-core CPU; on a multicore system there are even more ways for unsynchronized accesses to go wrong, since the CPUs have to co-ordinate their local and shared memory-caches correctly, which they won't know to do if there is no synchronization code included.

To sum up: Neither std::unordered_map nor std::string are designed for unsynchronized multithreaded access, and if you try to get away with it you're likely to regret it later on.

CodePudding user response：

Here's what I would do, if and only if I'm threading and there's a chance other threads are manipulating the list and its contents.

I would create a mutex lock when manipulating the list (which you've done).

And if I felt it was necessary to protect an individual item in the list (you're calling methods on it), I'd give each one a distinct mutex. You could change element A and element B simultaneously and it's fine, but by using the local locks for each item, each is safe.

However, it's very rare I've had to be that careful.