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

• Loki Astari: You do need to make your code correct for self moving potentially. This does not men you need a test. The standard swaping idium works without a need for a test.

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;

• DNKpp: I have to use both the const ref and non-const ref ctor because if I just use the const ref (as usual) the templated ctor becomes the better match if I try to copy a non-const object.
• Incomputable: @DNK, use sfinae. Writing another constructor is not a good idea.
• DNKpp: @Incomputable if you have an idea for that, pls show it to me. I didn't have one, so I used this approach
• Incomputable: @DNK, it is straightforward std::enable_if_t. I recommend you to try yourself, as this is the simplest form of SFINAE.
• DNKpp: @Incomputable as you can see in the code above, I am able to use std::enable_if. The question is about the specific problem for the (copy)ctors. I wasn't able to make a correct type_trait expression to get what I need.

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.