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

Question

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?

score 1 · Answer 1 · answered Jul 16 '20 at 01:19

1

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;
}

answered Jul 16 '20 at 01:19

Galik

47,303
4
80
117

You may be right. As @Human-Compiler points out, moving from a shared thing is dicy/tricky. Let's suppose I want to have exactly the safety I would have if `m_cache` weren't evaluated lazily and instead were set in the c'tor. In that case, I think(?) the move-c'tor doesn't have to lock `other.m_mutex` in that moving `m_cache` would be unsafe in the non-lazy version: If `m_cache` were set in the c'tor, then the move-c'tor would be unsafe if the other thread is reading it while we move it. I think(?) that's the same unsafety as having no lock in the move c'tor in the lazy-evaluation case? – Ben Jul 16 '20 at 13:39
@Ben To be honest I don't think I particularly agree with Human-Compiler's assessment. You will always have to consider *move semantics* in multi-threaded programs because half the standard library is movable. You will frequently be dealing with objects that are potentially being moved from by more than one thread or being read wile being moved from if not protected. What this answer does is avoid those *race conditions* when moving. But whether or not it is meaningful or well-defined to use an object after moving (by any thread) is up to the program logic and independent of threading. – Galik Jul 16 '20 at 14:13
@Ben It is necessary to lock the `other.m_cache` in the move constructor because you can not know it the `other` object is being updated by another thread. As you point out the other thread could be trying to read from the object you are trying to *move from*. This solution will prevent a race condition in that scenario. – Galik Jul 16 '20 at 14:14
But, if our definition of safety is "as safe as if we had calculated things up front", then isn't "you can not know if the `other` object is being [read] by another thread" just as UB? In either case I'd need another layer of logic to ensure one thread isn't reading `other` while we are moving from it? (I'll edit my question to include an uncached field.) – Ben Jul 16 '20 at 14:58
@Ben To be honest I was just looking at the copy/move mechanics of the internal object. I did not realize you make a `const T&` available to all and sundry. I think this answer does make the lazy solution as safe as the constructed solution but neither is safe in the event of another thread accessing that `const T& getExpensiveResult()`. The only solution I can think to offer to that is to make the object's internal available externally so other threads can synchronize on the same mutex. – Galik Jul 16 '20 at 15:13
@Ben Something like a `std::mutex& mutex() { return m_mutex; }` or perhaps something like this https://stackoverflow.com/questions/37979760/using-a-mutex-to-block-execution-from-outside-the-critical-section/37980869#37980869 – Galik Jul 16 '20 at 15:14

score 1 · Answer 2 · answered Jul 16 '20 at 01:33

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.

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

2 Answers2