CODEWITHSUNDEEP
c++move-semantics
Ben
Ben

Reputation: 9723

Best practices for move constructor of a class with mutex-locked caching

I sometimes have classes like this:

class HasMutexLockedCache {
private:
    SomeType m_regularData;
    mutable std::mutex m_mutex;
    mutable std::optaional<T> m_cache;
    // Etc.

public:
    const m_regularData& getData() const { return m_regularData; }
    
    const T& getExpensiveResult() const {
        std::scoped_lock lock(m_mutex);
        if (!m_cache) {
            m_cache = expensiveFunction();
        }
        return m_cache;
    }
    HasMutexLockedCache(const HasMutexLockedCache& other) 
        : m_regularData(other.m_regularData) 
    {
        std::scoped_lock lock(other.m_mutex);
        // I figure we don't have to lock this->m_mutex because
        // we are in the c'tor and so nobody else could possibly
        // be messing with m_cache.
        m_cache = other.m_cache;
    }
    HasMutexLockedCache(HasMutexLockedCache&& other)
        : m_regularData(std::move(other.m_regularData))
    {
        // What here?
    }
    HasMutexLockedCache& operator=(HasMutexLockedCache&& other) {
        m_regularData = std::move(other.m_regularData);
        // Bonus points: What here? Lock both mutexes? One? 
        // Only lock this->m_mutex depending on how we 
        // document thread safety?
    }
};

My question: what goes in HasMutexLockedCache(HasMutexLockedCache&& other) (and likewise in HasMutexLockedCache& operator=(HasMutexLockedCache&& other)? I think we don't need to lock other.m_mutex because, for other to be an rvalue reference, we know nobody else can see it, just like we don't have to lock this->m_mutex in the c'tor. However, I'd like some guidance. What are the best practices here? Should we be locking other.m_mutex?

Upvotes: 1

Views: 140

Answers (2)

Bitwize
Bitwize

Reputation: 11230

I'd like some guidance. What are the best practices here? Should we be locking other.m_mutex?

@Galik's answer explains how one could implement a move-constructor for this, however you should consider whether this is a safe and coherent idea for your abstraction.

If an object contains a std::mutex, generally this means that it may have concurrent accesses at different times which warrant this. If this is the case, then move-semantics can be quite difficult to work with when faced with multi-threading -- since you may have thread A move the contents of m_cache before thread B accesses it, resulting in it reading a moved-from state (which, depending on what the state is being checked, may not be well-defined). These types of errors can be quite complicated to debug, and even harder to reproduce!

Often if you have a type like this, it would be better to be sharing this type across threads explicitly, either with a shared lifetime via shared_ptr, or some form of synchronization from outside so each thread cannot destructively interfere with one another.

Upvotes: 1

Galik
Galik

Reputation: 48635

You have to remember that l-value objects can be moved using std::move so we do need to lock them too:

HasMutexLockedCache(HasMutexLockedCache&& other) {
    std::scoped_lock lock(other.m_mutex);
    m_cache = std::move(other.m_cache);
}

HasMutexLockedCache& operator=(HasMutexLockedCache&& other) {
    std::scoped_lock lock(m_mutex, other.m_mutex);
    m_cache = std::move(other.m_cache);
    return *this;
}

Upvotes: 1

Related Questions