Synchronized template wrapper class

Question

Issue: Code base has lots of data structures which are accessed between threads with >= 1 writer. Application logic becomes obfuscated due to lots of mutex locks.

Solution: Create a template class which provides synchronized access through member functions.

This seems very hacky -- Is this a futile effort / antipattern?

#ifndef SYNCHRONIZED_HPP_
#define SYNCHRONIZED_HPP_

#include <functional>
#include <mutex>
#include <shared_mutex>
#include <type_traits>
#include <utility>


#undef DISALLOW_EVIL_CONSTRUCTORS
#define DISALLOW_EVIL_CONSTRUCTORS(TypeName) \
  TypeName(const TypeName&);                 \
  void operator=(const TypeName&)


template <typename T>
class synchronized;

template <typename T> auto
make_synchronized(T&& value) {
  return synchronized<T>{ std::forward<T>(value) };
}

template <typename T>
class synchronized final {
 public:
  using value_t = std::remove_reference_t<T>;

  template <typename... Args>
  explicit synchronized(Args&&... args)
    : value_(std::forward<Args>(args)...)
  {}

  value_t get() const {
    read_lock l(mutex_);
    return value_;
  }
  
  template <typename U> void
  set(U&& new_value) {
    write_lock l(mutex_);
    value_ = std::forward<U>(new_value);
  }
  
  template <typename Accessor>
  void use(Accessor&& access) const {
    read_lock l(mutex_);
    std::forward<Accessor>(access)(value_);
  }

  template <typename Mutator>
  void alter(Mutator&& func) {
    write_lock l(mutex_);
    std::forward<Mutator>(func)(value_);
  }

 private:
  value_t value_;
  using mutex_t = std::shared_timed_mutex;
  using read_lock = std::shared_lock<mutex_t>;
  using write_lock = std::unique_lock<mutex_t>;
  mutable mutex_t mutex_{};

  DISALLOW_EVIL_CONSTRUCTORS(synchronized);
};


#endif // SYNCHRONIZED_HPP_

With doxy comments (same code):

#ifndef SYNCHRONIZED_HPP_
#define SYNCHRONIZED_HPP_

#include <functional>
#include <mutex>
#include <shared_mutex>
#include <type_traits>
#include <utility>


#undef DISALLOW_EVIL_CONSTRUCTORS
#define DISALLOW_EVIL_CONSTRUCTORS(TypeName) \
  TypeName(const TypeName&);                 \
  void operator=(const TypeName&)





template <typename T>
class synchronized;


/**
* @brief
*    Make a synchronized<T> using template deduction.
*
* @sa synchronized
*/
template <typename T> auto
make_synchronized(T&& value) {
  return synchronized<T>{ std::forward<T>(value) };
}

/**
* @brief
*    Provides straightforward thread-synchronized access to template type.
*
* @note
*    While access to the immediate type is synchronized, this class does not
*  prevent non-synchronized access of pointer or reference members of the
*  template type.
*
* @note
*    For applicable types, prefer std::atomic<T>.
*/
template <typename T>
class synchronized final {
 public:
  using value_t = std::remove_reference_t<T>;

  template <typename... Args>
  explicit synchronized(Args&&... args)
    : value_(std::forward<Args>(args)...)
  {}

  /**
  * @brief
  *    Thread-sychronized get.
  */
  value_t get() const {
    read_lock l(mutex_);
    return value_;
  }

  /**
  * @brief
  *    Thread-sychronized set.
  */
  template <typename U> void
  set(U&& new_value) {
    write_lock l(mutex_);
    value_ = std::forward<U>(new_value);
  }

  /**
  * @brief
  *    Use the underlying value where get() would be less-trivial or
  *  otherwise unsuitable.
  *
  * @note
  *    Prefer get(), especially if this access takes a long time, as to not
  *  starve a writer thread or other reader threads.
  */
  template <typename Accessor>
  void use(Accessor&& access) const {
    read_lock l(mutex_);
    std::forward<Accessor>(access)(value_);
  }

  /**
  * @brief
  *    Alter (mutate) the underlying value where get() and set() would be
  *  less-trivial or otherwise unsuitable.
  *
  * @note
  *    Prefer the combination of get() then set(), especially if this access
  *  takes a long time, as to not starve other writer or reader threads.
  */
  template <typename Mutator>
  void alter(Mutator&& func) {
    write_lock l(mutex_);
    std::forward<Mutator>(func)(value_);
  }

 private:
  value_t value_;
  using mutex_t = std::shared_timed_mutex;
  using read_lock = std::shared_lock<mutex_t>;
  using write_lock = std::unique_lock<mutex_t>;
  mutable mutex_t mutex_{};

  DISALLOW_EVIL_CONSTRUCTORS(synchronized);
};


#endif // SYNCHRONIZED_HPP_

Answer 1

Seems like a decent implementation. You should be aware that nothing in this area is foolproof. Two ways to "break" your code are:

auto si = make_synchronized(42);
// ...
int *one;
si.alter([&](int& val) {
    one = &val;
});
*one = 42;  // use outside of the lock

and

auto si = make_synchronized(42);
auto& si2 = si;
// ...
si.use([&](const int&) {
    si2.alter([&](int&) {
        // deadlock
    });
});

But this is no big deal, as long as you trust your users (which might mean yourself!) to avoid getting into this kind of situation. The second situation above (deadlock) is particularly easy to hide by placing the second si.alter inside a helper function, or by aliasing the way I did with si2.

---

#undef DISALLOW_EVIL_CONSTRUCTORS
#define DISALLOW_EVIL_CONSTRUCTORS(TypeName) \
  TypeName(const TypeName&);                 \
  void operator=(const TypeName&)

At least three issues here. First, you should be using =delete instead of this macro. Simply:

template <typename T>
class synchronized final {
public:
    synchronized(synchronized&&) = delete;
    synchronized(const synchronized&) = delete;
    synchronized& operator=(synchronized&&) = delete;
    synchronized& operator=(const synchronized&) = delete;

(Once you've explicitly =deleted the move operations, further =deleteing the copy operations is unnecessary... but recommended, for clarity.)

Second, although it doesn't apply too much in this specific case, having a macro that consists of multiple statements or declarations is usually a red flag. Look up "hygienic macros in C" for more information on the do { ... } while (0) idiom.

Third, you're using

#undef X
#define X Y

as a way to say "I really really want X to mean Y, even if someone else has already defined X to mean something different." That's probably a bad idea. If nobody else has previously defined X, then your #undef is redundant (and should be removed). If someone else has previously defined X, then your #undef makes your own code work but presumably breaks whatever that person was using X for! Rather than redefine X in a way that might break their code down the line, it would be much much better to just error out right here in that case (so again, the solution is to remove the #undef).

---

I notice the final keyword on class synchronized. Why is it important to you that this class be final?

---

template <typename Accessor>
void use(Accessor&& access) const {
  read_lock l(mutex_);
  std::forward<Accessor>(access)(value_);
}

Perfect forwarding here is fine, but almost certainly overkill. I can't think of any case where I'd want the accessor itself to be either non-const or an rvalue. The accessor should always be of the form [&](const auto& value) { ... }; it never needs to have any captures that could be affected by mutable, and thus it never needs to be called as non-const. So we can simplify:

template<class Accessor>
void use(const Accessor& access) const {
    read_lock l(mutex_);
    access(value_);
}

(Incidentally, anything other than four-space indents is horrible. :))

---

template <typename T> auto
make_synchronized(T&& value) {
  return synchronized<T>{ std::forward<T>(value) };
}

Looks good, but...

• The auto here isn't buying you anything; it might be better to give the return type explicitly as synchronized<T>.

• The curly braces in synchronized<T>{ ... } are inconsistent with the way make_ functions work everywhere else in the Standard Library. make_shared uses parens. make_unique uses parens. To observe the difference, make a class A where A{...} and A(...) call different constructors, like this. (make_pair and make_tuple also use parens, although I'm not sure if the difference is observable in those two cases.)