3
\$\begingroup\$

I'm currently trying to write useful helper classes for easier concurrent programming in C++ while at the same time also teaching myself 'modern C++'.

This is supposed to be a synchronized object class template, which essentially means that it wraps an object and it's supposed to guarantee that if the owner of the object hasn't gone out of his/her way to screw things up then the object should be thread safe.

It supports two kinds of operations. The first is the overloaded arrow operator which locks the object, performs an action and then unlocks it. The second one is the dereference operator that returns a handle that is a 'safe' reference to the object.

It's essentially supposed to be an easy way of making almost any object thread safe.

#pragma once

#ifndef SDK_SYNCHRONIZED_HPP
#define SDK_SYNCHRONIZED_HPP


#include <type_traits>
#include <mutex>


namespace sdk {

    template<typename T, typename Mutex = std::recursive_mutex>
    class synchronized {
        class object {
        public:

            object(T* obj, Mutex& lock) noexcept
                : m_obj(obj), m_lock(lock) {
            }

            inline object(object&& that) noexcept
                : m_obj(std::move(that.m_obj)), m_lock(std::move(that.m_lock)) {
            }

            inline auto operator->() noexcept {
                return m_obj;
            }

            inline auto operator->() const noexcept {
                return m_obj;
            }

        private:
            T* m_obj;
            std::unique_lock<Mutex> m_lock;
        };

    public:

        template<typename ... A,
         typename = std::enable_if_t<std::is_constructible_v<T, A ...>>>
        synchronized(A&& ... args)
            : m_mutex(), m_obj { std::forward<A>(args) ... } {
        }

        template<typename ... A,
         typename = std::enable_if_t<std::is_constructible_v<T, A ...>>>
        synchronized(Mutex&& m, A&& ... args)
            : m_mutex(m), m_obj { std::forward<A>(args) ... } {
        }

        synchronized(synchronized&& that)
            : m_mutex(std::move(that.m_mutex)), m_obj(std::move(m_obj)) {
        }

        // used for several actions

        [[nodiscard]] inline object operator*() {
            return object(&m_obj, m_mutex);
        }

        [[nodiscard]] inline const object operator*() const {
            return object(&m_obj, m_mutex);
        }

        // synchronized operations

        inline auto operator->() {
            object obj(&m_obj, m_mutex);
            return obj;
        }

        inline auto operator->() const {
            object obj(&m_obj, m_mutex);
            return obj;
        }

    private:

        Mutex m_mutex;
        T m_obj;

    };
}

#endif

And the usage is (normally) supposed to be something like:

// this is supposed to be safe

synchronized<Foo> foo(1, 2.f, true);
// alot of other stuff
auto dostuff = std::thread([&foo](){
    // more stuff here
    foo->bar(false);
    // you get the idea
});
//...
foo->bar(true);
\$\endgroup\$
1
  • \$\begingroup\$ Look up std::promise and std::future and std::async \$\endgroup\$ Oct 19, 2017 at 22:23

1 Answer 1

2
\$\begingroup\$

Correctness

foo can be destructed while another thread is performing an operation on it, because the destructor or its caller don't have to acquire a lock to do so. Technically destroying a mutex while it is locked already is undefined behavior, but it gets worse if another thread happens to modify the now invalid object.

Same reasoning for the move constructor: The object from which will be moved could still be modified concurrently by another thread, so locking would be required.

Also, rule of 3/5 violation: There's a custom move constructor (and there will hopefully be a corrected destructor), so copy constructor, move assignment operator and copy assignment operator should also be provided (currently, they copy without locking neither this nor the other object).

Also, m_mutex should be marked mutable, otherwise a const synchronized<Foo>& can't be accessed.

Other nitpicks

  • In the second constructor, m should be moved.
  • object::object(T*, Mutex&) should probably take it's first argument as T& instead, as it should never be nullptr.
  • object as a name is very broad. Maybe change it to something more concise, like with_lock or locked_handle?

Suggestion

If targeting C++17 or later, consider providing a partial specialization for std::shared_mutex: const access paths only require a std::shared_lock(read only), while non-const access paths should be locked with std::unique_lock (write access). Also, for convenience, a as_const() member function to explicitly acquire a const reference would help. See this talk by Andrei Alexandrescu for more information on that topic.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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