LockedValue implementation

Question

I am working on class for simple sharing values between threads without race-conditions, which can't be a std::atomic (this means, no POD and no trivially-copyable types). The initial intention was to lock a std::function against writing while reading from an other thread.

The class itself is simple; just lock a mutex at every sensitive part of the class, but I am not totally sure if it's correct or not. I think about to lock the mutex in the dtor, but I don't know if it's good style or not.

#pragma once

template <class T>
class LockedValue
{
private:
    using Lock = std::scoped_lock<std::mutex>;

public:
    LockedValue() = default;

    LockedValue(const LockedValue& _other) :
        m_Value(_other.load())
    {}

    LockedValue(LockedValue& _other) :
        LockedValue(static_cast<const LockedValue&>(_other))
    {}

    LockedValue(LockedValue&& _other) :
        m_Value(_other._takeValue())
    {}

    template <class... Args>
    LockedValue(Args&&... _args) :
        m_Value(std::forward<Args>(_args)...)
    {}

    LockedValue& operator =(const LockedValue& _other)
    {
        auto value = _other.load();
        store(std::move(value));
        return *this;
    }

    LockedValue& operator =(LockedValue&& _other)
    {
        if (this != &_other)
            store(_other._takeValue());
        return *this;
    }

    template <class T_, typename = std::enable_if_t<std::is_convertible_v<T_, T>>>
    LockedValue& operator =(T_&& _value)
    {
        store(std::forward<T_>(_value));
        return *this;
    }

    T operator *() const
    {
        return load();
    }

    template <class T_>
    void store(T_&& _value)
    {
        Lock lock(m_Mutex);
        m_Value = std::forward<T_>(_value);
    }

    T load() const
    {
        Lock lock(m_Mutex);
        return m_Value;
    }

private:
    mutable std::mutex m_Mutex;
    T m_Value;

    T&& _takeValue()
    {
        Lock lock(m_Mutex);
        return std::move(m_Value);
    }
};

EDIT: Is it necessary to check for self-moving in move assign? I think it should be ok to remove the self check.

EDIT2: To the copy ctor related discussion. Delete the non-const& copy ctor from the class and try to use an object like this.

If you don't have c++17 change the Lock alias to std::lock_guard and change std::is_convertible_v to std::is_convertible::value

I pasted it at this online compiler with the ctor already deactivated click me

// calls the variadic ctor
LockedValue<std::string> s("test");

// this is fine; calls the usual copty ctor
auto u(static_cast<const LockedValue<std::string>&>(s)); 

/* results in an error because type LockedValue<std::string>&
can not be converted to std::string; this means a second call
for the variadic ctor*/
auto t(s);

Answer 1

You don't need both version of the copy constructor.

    LockedValue(const LockedValue& _other) :
        m_Value(_other.load())
    {}

    LockedValue(LockedValue& _other) :
        LockedValue(static_cast<const LockedValue&>(_other))
    {}

If this is a mutating copy constructor. Then just have the second one. Otherwise have the first one. Since the mutating one simple calls the const version I see no reason for the second one as a non const object will still bind to the const version.

Move operations are usually noexcept. I suppose your are potentially throwing. I would need to look up all the funtions called. You should check and if your move operations are non throwing then you need to make an appropriate comment.

    LockedValue(LockedValue&& _other) :
        m_Value(_other._takeValue())
    {}

A standard move is simply a swap. I suppose you would need a safe swap. Which would be doubly hard as you run into race conditions. But there is always std::lock to help you with that (But its an extra step).

    LockedValue& operator =(LockedValue&& _other)
    {
        if (this != &_other)
            store(_other._takeValue());
        return *this;
    }

Thay's nice. I think I learned a new thing for the day.

    template <class T_, typename = std::enable_if_t<std::is_convertible_v<T_, T>>>
    LockedValue& operator =(T_&& _value)
    {
        store(std::forward<T_>(_value));
        return *this;
    }

And a correct usage of the mutable keyword.

    mutable std::mutex m_Mutex;

Answer 2

First of all, thanks for the feedback. I answer my post myself, because I have to describe my decisions.

First of all, here the current code.

#pragma once

template <class T>
class SynchronizedValue
{
private:
    using Lock = std::scoped_lock<std::mutex>;

public:
    SynchronizedValue() = default;

    SynchronizedValue(const SynchronizedValue& _other) :
        m_Value(_other.load())
    {}

    SynchronizedValue(SynchronizedValue& _other) :
        SynchronizedValue(static_cast<const SynchronizedValue&>(_other))
    {}

    SynchronizedValue(SynchronizedValue&& _other) :
        m_Value(_other._takeValue())
    {}

    template <typename... Args>
    SynchronizedValue(Args&&... _args) :
        m_Value(std::forward<Args>(_args)...)
    {}

    SynchronizedValue& operator =(const SynchronizedValue& _other)
    {
        store(_other.load());
        return *this;
    }

    SynchronizedValue& operator =(SynchronizedValue&& _other)
    {
        store(_other._takeValue());
        return *this;
    }

    template <class U, typename = std::enable_if_t<std::is_convertible_v<U, T>>>
    SynchronizedValue& operator =(U&& _value)
    {
        store(std::forward<U>(_value));
        return *this;
    }

    T operator *() const
    {
        return load();
    }

    template <class U>
    void store(U&& _value)
    {
        Lock lock(m_Mutex);
        m_Value = std::forward<U>(_value);
    }

    T load() const
    {
        Lock lock(m_Mutex);
        return m_Value;
    }

private:
    mutable std::mutex m_Mutex;
    T m_Value;

    T _takeValue()
    {
        Lock lock(m_Mutex);
        return std::move(m_Value);
    }
};

As you can see, I renamed my class to SynchronizedValue, because I find it more descriptive than LockedValue. The behaviour of the class stays unchanged, I only did some minor changes and I want to describe why.

In every state of this class, only one mutex gets locked. Even in the move parts, I only had to lock one at the same time. This prevents me from getting into a deadlock. The real important part is in the T _takeValue() function. In the last state of this class I returned a rvalue reference, which was bad, because when I really started to move the value from other, outside of the function, the mutex already was getting unlocked. This means a race-condition could occur, which is absolutely bad. To fix this, my only change here is to return a real object instead of a rvalue reference. The second important change is no code change at all. It is also closely related to the _takeValue() function. I am talking about the move assignment operator. While I had to check the self assign in the previous version, I can now just take the value and store it safely. The cleanup, which is the bad thing of a self-move, already happened inside of the _takeValue() function. Thus, it should be absolutely safe to deal with the self-moving problem.

Why no noexcept

The lock functions of a std::mutex might throw, this means I am not able to make a guarantee for exception safety, unless I would write the whole function inside of a try/catch block, which is never a good idea to deal with exceptions. It is not a mistake to declare a move-assign not noexcept; it just makes some optimizations for some std classes impossible.

why a const& and non-const& copy ctor

That's a good question. While I know, that usually a const& copy ctor is enough, unless you have to modify the original object, it is here required to make the templated ctor possible. If I delete the non-const version of the copy ctor, everything works fine, unless I try to copy from a non-const SynchronizedValue object. A conversion from & to const& is required; and a templated overloading always has a closer match than an overloading which requires a conversation; even when it's just the const to a reference. I played a little bit around with SFINAE, but I got no nice solution for that (there might be a solution; but I wasn't able to make it work), thus I used this easy way to deal with this issue. The problem is, I can't SFINAE the copy ctor, this is the wrong way. I have to remove the templated ctor from the match list when I want to copy; and that's the real issue here. The idea, which someone came up with, was to try this one:

template <typename... Args, typename = std::enable_if_t<sizeof...(Args) != 1 || (!std::is_same_v<std:.decay_t<Args>, LockedValue> && ...)>>

but like I said, I wasn't able to get this work.