2
\$\begingroup\$

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);
\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – Martin York Dec 12 '17 at 21:46
2
\$\begingroup\$

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;
\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – DNKpp Dec 12 '17 at 22:03
  • \$\begingroup\$ @DNK, use sfinae. Writing another constructor is not a good idea. \$\endgroup\$ – Incomputable Dec 12 '17 at 22:22
  • \$\begingroup\$ @Incomputable if you have an idea for that, pls show it to me. I didn't have one, so I used this approach \$\endgroup\$ – DNKpp Dec 12 '17 at 22:27
  • \$\begingroup\$ @DNK, it is straightforward std::enable_if_t. I recommend you to try yourself, as this is the simplest form of SFINAE. \$\endgroup\$ – Incomputable Dec 12 '17 at 22:28
  • \$\begingroup\$ @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. \$\endgroup\$ – DNKpp Dec 12 '17 at 22:30
0
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.