15

I need to make a thread-safe map, where I mean that each value must be independently mutexed. For example, I need to be able to get map["abc"] and map["vf"] at the same time from 2 different threads.

My idea is to make two maps: one for data and one for mutex for every key:

class cache
{
private:
....

    std::map<std::string, std::string> mainCache;
    std::map<std::string, std::unique_ptr<std::mutex> > mutexCache;
    std::mutex gMutex;
.....
public:
    std::string get(std::string key);

};
std::string cache::get(std::string key){
    std::mutex *m;
    gMutex.lock();
    if (mutexCache.count(key) == 0){
        mutexCache.insert(new std::unique_ptr<std::mutex>);
    }
    m = mutexCache[key];
    gMutex.unlock();
}

I find that I can't create map from string to mutex, because there is no copy constructor in std::mutex and I must use std::unique_ptr; but when I compile this I get:

/home/user/test/cache.cpp:7: error: no matching function for call to 'std::map<std::basic_string<char>, std::unique_ptr<std::mutex> >::insert(std::unique_ptr<std::mutex>*)'
         mutexCache.insert(new std::unique_ptr<std::mutex>);
                                                          ^

How do I solve this problem?

Toby Speight
  • 27,591
  • 48
  • 66
  • 103
Mike Minaev
  • 1,912
  • 4
  • 23
  • 33

4 Answers4

21

TL;DR: just use operator [] like std::map<std::string, std::mutex> map; map[filename];

Why do you need to use an std::unique_ptr in the first place?

I had the same problem when I had to create an std::map of std::mutex objects. The issue is that std::mutex is neither copyable nor movable, so I needed to construct it "in place".

I couldn't just use emplace because it doesn't work directly for default-constructed values. There is an option to use std::piecewise_construct like that:

map.emplace(std::piecewise_construct, std::make_tuple(key), std::make_tuple());

but it's IMO complicated and less readable.

My solution is much simpler - just use the operator[] - it will create the value using its default constructor and return a reference to it. Or it will just find and return a reference to the already existing item without creating a new one.

std::map<std::string, std::mutex> map;

std::mutex& GetMutexForFile(const std::string& filename)
{
    return map[filename]; // constructs it inside the map if doesn't exist
}
Roman Kruglov
  • 3,375
  • 2
  • 40
  • 46
  • 1
    Since the `[]` operator of `std::map` is not thread-safe, you need to lock in `GetMutexForFile` – Nimrod Dec 27 '21 at 05:30
  • @Nimrod Why would he need to lock? He likely just wants to read the reference of the mutex, and will not change it's value. Therefore there shouldn't be any inconsistencies. – Daniel Mar 24 '22 at 10:06
  • 2
    @Daniel Since `[]` will insert one when not exists. Inserting for `std::map` is not thread-safe. Under multiple threads, you can't promise the insert succeeds and the result is valid. – Nimrod Mar 25 '22 at 06:19
  • 1
    @Nimrod Right. Since my last comment I read some more on the topic, and realized the problem. The implementation of the std::map is an AVL tree, which have to be rebalanced times to times, which is causing the issue with thread safety. Here is a closely related question, which I recommend everybody interested to have a look: https://stackoverflow.com/a/27829837/12664906 – Daniel Mar 25 '22 at 09:01
16

Replace mutexCache.insert(new std::unique_ptr<std::mutex>) with:

mutexCache.emplace(key, new std::mutex);

In C++14, you should say:

mutexCache.emplace(key, std::make_unique<std::mutex>());

The overall code is very noisy and inelegant, though. It should probably look like this:

std::string cache::get(std::string key)
{
    std::mutex * inner_mutex;

    {
        std::lock_guard<std::mutex> g_lk(gMutex);

        auto it = mutexCache.find(key);
        if (it == mutexCache.end())
        {
            it = mutexCache.emplace(key, std::make_unique<std::mutex>()).first;
        }
        inner_mutex = it->second.get();
    }

    {
        std::lock_guard<std::mutex> c_lk(*inner_mutex);
        return mainCache[key];
    }
}
Jiří Pospíšil
  • 14,296
  • 2
  • 41
  • 52
Kerrek SB
  • 464,522
  • 92
  • 875
  • 1,084
0

If you have access to c++17, you can use std::map::try_emplace instead of using pointers and it should work just fine for non-copyable and non-movable types!

Lectem
  • 503
  • 6
  • 19
0

Your mutexes actually don't protect values. They are released before returning from get, and then other thread can get referrence to the same string second time. Oh, but your cache returns copies of strings, not references. So, there is no point in protecting each string with own mutex.

If you want to protect cache class from concurrent access only gMutex is sufficient. Code should be

class cache
{
private:

    std::map<std::string, std::string> mainCache;
    std::mutex gMutex;

public:
    std::string get(const std::string & key);
    void set(const std::string & key, const std::string & value);

};

std::string cache::get(const std::string & key) {
    std::lock_guard<std::mutex> g_lk(gMutex);
    return mainCache[key];
}

void cache::set(const std::string & key, const std::string & value) {
    std::lock_guard<std::mutex> g_lk(gMutex);
    mainCache[key] = value;
}

If you want to provide a way for many threads to work concurrently with string instances inside your map and protect them from concurrent access things become more tricky. First, you need to know when thread finished to work with string and release the lock. Otherwise once accessed value becomes locked forever and no other thread can access it.

As possible solution you can use some class like

#include <iostream>
#include <string>
#include <map>
#include <mutex>
#include <memory>


template<class T>
class SharedObject {
private:
    T obj;
    std::mutex m;

public:
    SharedObject() = default;
    SharedObject(const T & object): obj(object) {}
    SharedObject(T && object): obj(std::move(object)) {}

    template<class F>
    void access(F && f) {
        std::lock_guard<std::mutex> lock(m);
        f(obj);
    }
};


class ThreadSafeCache
{
private:

    std::map<std::string, std::shared_ptr<SharedObject<std::string>>> mainCache;
    std::mutex gMutex;

public:
    std::shared_ptr<SharedObject<std::string>> & get(const std::string & key) {
        std::lock_guard<std::mutex> g_lk(gMutex);
        return mainCache[key];
    }
    void set(const std::string & key, const std::string & value) {
        std::shared_ptr<SharedObject<std::string>> obj;
        bool alreadyAssigned = false;
        {
            std::lock_guard<std::mutex> g_lk(gMutex);
            auto it = mainCache.find(key);
            if (it != mainCache.end()) {
                obj = (*it).second;
            }
            else {
                obj = mainCache.emplace(key, std::make_shared<SharedObject<std::string>>(value)).first->second;
                alreadyAssigned = true;
            }
        }
        // we can't be sure someone not doing some long transaction with this object,
        // so we can't do access under gMutex, because it locks all accesses to all other elements of cache
        if (!alreadyAssigned) obj->access([&value] (std::string& s) { s = value; });
    }

};

// in some thread
void foo(ThreadSafeCache & c) {
    auto & sharedString = c.get("abc");
    sharedString->access([&] (std::string& s) {
        // code that use string goes here
        std::cout << s;
        // c.get("abc")->access([](auto & s) { std::cout << s; }); // deadlock
    });
}

int main()
{
  ThreadSafeCache c;
  c.set("abc", "val");
  foo(c);
  return 0;
}

Of course, real implementation of these classes should have more methods providing more complex semantic, take const-ness into account and so on. But I hope main idea is clear.

EDIT:

Note: shared_ptr to SharedObject should be used because you can't delete mutex while lock is held, so there is no way of safe deleting map entries if value type is SharedObject itself.

aaalex88
  • 619
  • 4
  • 13