Automatic RAII wrapper for concurrent access

Question

Probably many people had to work with multithreaded applications with C++ and can understand how messy can be fine-grained locking of objects.

So once in a while I came to idea of implementing some proxy objects using a so-called "drill down" property of operator-> and RAII in mind.

Yes. There is some implementations that do something like that but I didn't find something that fully satisfy me or at least something battle tested but not bloated.

Here I want to show and discuss working prototype for shared locking of objects that works as simple as that:

<...>
class dummy {
 public:
 dummy(std::string p) : p_(p) { }
 std::string to_string() const { return p_; }
 void set_string(std::string str) { p_ = str; }
 private:
 std::string p_;
};
<...>
// there it is
shared_lock_wrap<dummy> data("hello");
{
 auto p = data.get();
 // p->set_string("hello, world"); // will cause an error
 std::cout << "ro access: " << p->to_string() << std::endl;
}
auto p = data.get_mutable();
p->set_string("hello, stackexchange");
std::cout << "rw access: " << p->to_string() << std::endl;
<...>

If you're still interested there is implementation of shared_lock_wrap that was used earlier. This should work with any decent C++14 compiler (I used GCC 8.2):

template<
 typename T,
 typename _TMtx,
 template<typename> class _TLock,
 template<typename> class _TMutLock
>
class lock_wrap_impl {
 template<typename W>
 using mimic_unique = typename std::conditional<
 std::is_move_constructible<W>::value,
 W,
 std::unique_ptr<W>
 >::type;
 using mutex_type = std::unique_ptr<_TMtx>;
 using lock_type = mimic_unique<_TLock<_TMtx> >;
 using mutable_lock_type = mimic_unique<_TMutLock<_TMtx> >;
 template<bool M, typename W>
 using mimic_const = typename std::conditional<M, W, const W>::type;
 template<typename L>
 using is_mutable = typename std::is_same<L, mutable_lock_type>;
 template<bool M=true>
 using ptr_type = const std::shared_ptr<mimic_const<M, T> >;
 public:
 template<typename L>
 class proxy {
 public:
 ptr_type<is_mutable<L>::value > operator->() { return obj_; }
 proxy(proxy<L>&& other) :
 obj_(other.obj_),
 lock_(std::move(other.lock_))
 {}
 private:
 friend lock_wrap_impl<T, _TMtx, _TLock, _TMutLock>;
 proxy(ptr_type<> obj, L&& lock) :
 obj_(obj),
 lock_(std::move(lock))
 {}
 ptr_type<> obj_;
 L lock_;
 };
 lock_wrap_impl(T&& obj) :
 obj_(&obj),
 mtx_(mutex_type(new _TMtx))
 {}
 template<typename ...Args>
 lock_wrap_impl(Args&& ...args) :
 obj_(new T(std::forward<Args>(args)...)),
 mtx_(mutex_type(new _TMtx))
 {}
 lock_wrap_impl(lock_wrap_impl<T, _TMtx, _TLock, _TMutLock>&& other) :
 obj_(std::move(other.obj_)),
 mtx_(std::move(other.mtx_))
 {}
 /**
 * For types that ARE move constructible
 * e.g std::shared_lock
 */
 template<typename Q = _TLock<_TMtx> >
 proxy<lock_type> get(typename std::enable_if<std::is_move_constructible<Q>::value>::type* = 0) {
 return proxy<lock_type>(obj_, lock_type(*mtx_));
 }
 template<typename Q = _TMutLock<_TMtx> >
 proxy<mutable_lock_type> get_mutable(typename std::enable_if<std::is_move_constructible<Q>::value>::type* = 0) {
 return proxy<mutable_lock_type>(obj_, mutable_lock_type(mtx_));
 }
 /**
 * For types that aren't move constructible
 */
 template<typename Q = _TLock<_TMtx> >
 proxy<lock_type> get(typename std::enable_if<!std::is_move_constructible<Q>::value>::type* = 0) {
 return proxy<lock_type>(obj_, lock_type(new Q(*mtx_)));
 }
 template<typename Q = _TMutLock<_TMtx> >
 proxy<mutable_lock_type> get_mutable(typename std::enable_if<!std::is_move_constructible<Q>::value>::type* = 0) {
 return proxy<mutable_lock_type>(obj_, mutable_lock_type(new Q(*mtx_)));
 }
 private:
 ptr_type<> obj_;
 mutex_type mtx_;
};
template<typename T>
using shared_lock_wrap = lock_wrap_impl<T, std::shared_timed_mutex, std::shared_lock, std::unique_lock>;

Worth to note that I'm aware that there's some drawbacks: I prefer to use shared locks (e.g std::shared_mutex/shared_timed_mutex or boost alternatives) but kept in mind that there are mutexes/lockers types that are "exclusively lockable" (like std::lock_guard with std::mutex or boost::scoped_lock) but latter doesn't work with this exact implementation due to equality of _TLock and _TMutLock. Though it works fine with shared locks so maybe it will be fixed at some point but that is not what I personally needed.

So what do you guys think about it? About idea or code structure in general or basically anything that you think. Maybe you have some thoughts about how it can be optimized.

Personally I'm mostly prefer golang for developing but due to job or some embedded applications I also work with C++ or (occasionally plain C).

• papagaga: > but latter (like std::lock_guard) doesn't work due to equality of _TLock and _TMutLock.. Codereview is about reviewing working code, not fixing broken / incomplete code. Your comments mention std::lock_guard ( /* For types that aren't move constructible e.g. std::lock_guard*/) as a feature, and then you say it isn't working in your text. You should edit your code to retain only what's working (edit it before you get an answer).
• gudvinr: Well, I'm aware about that and did mention that main purpose was to provide interface for separate locking so it works as expected if you take it into consideration. Otherwise it may be frustrating if I just remove that comment and that note.
• gudvinr: @papagaga is it ok now? I changed description a little so it shouldn't be inappropriate anymore. Though I don't want to remove parts of the code (lock_guard still can be used as _TMutLock but just not with std::mutex)
• papagaga: It looks fine! It might very well have been fine since the beginning, but the wording was misleading.

Answer 1

template<
 typename T,
 typename _TMtx,

Get off the standard library's lawn! _Ugly names are reserved to the implementation.

 template<typename> class _TLock,
 template<typename> class _TMutLock
>
class lock_wrap_impl {
 template<typename W>
 using mimic_unique = typename std::conditional<
 std::is_move_constructible<W>::value,
 W,
 std::unique_ptr<W>
 >::type;

You tagged C++14, so std::conditional_t.

 using mutex_type = std::unique_ptr<_TMtx>;
 using lock_type = mimic_unique<_TLock<_TMtx> >;
 using mutable_lock_type = mimic_unique<_TMutLock<_TMtx> >;
 template<bool M, typename W>
 using mimic_const = typename std::conditional<M, W, const W>::type;
 template<typename L>
 using is_mutable = typename std::is_same<L, mutable_lock_type>;

This is a pointless typename.

 template<bool M=true>
 using ptr_type = const std::shared_ptr<mimic_const<M, T> >;

A more descriptive name for M may be a good idea. There's no reason to make this top-level const either.

 public:
 template<typename L>
 class proxy {
 public:
 ptr_type<is_mutable<L>::value > operator->() { return obj_; }

This should be a const member.

 proxy(proxy<L>&& other) :
 obj_(other.obj_),
 lock_(std::move(other.lock_))
 {}

This does nothing beyond what a defaulted move constructor does. Just default it. Also, use the injected-class-name: proxy(proxy&&) = default;

 private:
 friend lock_wrap_impl<T, _TMtx, _TLock, _TMutLock>;
 proxy(ptr_type<> obj, L&& lock) :
 obj_(obj),
 lock_(std::move(lock))
 {}
 ptr_type<> obj_;
 L lock_;
 };
 lock_wrap_impl(T&& obj) :
 obj_(&obj),
 mtx_(mutex_type(new _TMtx))
 {}

obj_(&obj) is badly wrong. You are making a shared_ptr to some random rvalue.

 template<typename ...Args>
 lock_wrap_impl(Args&& ...args) :
 obj_(new T(std::forward<Args>(args)...)),
 mtx_(mutex_type(new _TMtx))
 {}

This needs to be constrained. Also can use make_shared.

 lock_wrap_impl(lock_wrap_impl<T, _TMtx, _TLock, _TMutLock>&& other) :
 obj_(std::move(other.obj_)),
 mtx_(std::move(other.mtx_))
 {}

This again is just a defaulted move constructor. Additionally, since you made obj_ const, the first move is just a copy. Again, there's no need to make it const.

 /**
 * For types that ARE move constructible
 * e.g std::shared_lock
 */
 template<typename Q = _TLock<_TMtx> >
 proxy<lock_type> get(typename std::enable_if<std::is_move_constructible<Q>::value>::type* = 0) {
 return proxy<lock_type>(obj_, lock_type(*mtx_));
 }
 template<typename Q = _TMutLock<_TMtx> >
 proxy<mutable_lock_type> get_mutable(typename std::enable_if<std::is_move_constructible<Q>::value>::type* = 0) {
 return proxy<mutable_lock_type>(obj_, mutable_lock_type(mtx_));
 }
 /**
 * For types that aren't move constructible
 */
 template<typename Q = _TLock<_TMtx> >
 proxy<lock_type> get(typename std::enable_if<!std::is_move_constructible<Q>::value>::type* = 0) {
 return proxy<lock_type>(obj_, lock_type(new Q(*mtx_)));
 }
 template<typename Q = _TMutLock<_TMtx> >
 proxy<mutable_lock_type> get_mutable(typename std::enable_if<!std::is_move_constructible<Q>::value>::type* = 0) {
 return proxy<mutable_lock_type>(obj_, mutable_lock_type(new Q(*mtx_)));
 }

This whole business of wrapping immovable locks in unique_ptr is excessively costly and not useful in practice. If someone chose to use lock_guard, it's because they don't want to pay the difference between unique_lock and lock_guard, which is a stored bool and a branch in the destructor. That's far, far less than the cost of a trip to the heap.

 private:
 ptr_type<> obj_;
 mutex_type mtx_;
};

There does not appear to be any reason why this class need to be movable, and your move constructor leaves it in an unusable "emptier-than-empty" state.

---

Additional comments:

• Detecting mutable locks by type presupposes that the shared and mutable locks are not the same. There's no need to have that limitation, since your proxy can just be templated directly on something like bool Mutable instead.
• There's no reason to construct a lock and then move it into the proxy. Just pass the mutex and have the proxy's constructor construct the lock directly. You save a temporary, and it also plays well with C++17 by giving you support for immovable locks for free.
• There's also no reason to use a shared_ptr at all. The pointer obtained from -> cannot safely outlive the proxy (because you'd be accessing the object while not holding the lock) and the proxy cannot safely outlive the wrapper object (or you'd be destroying a locked mutex and then unlocking a destroyed mutex; both are UB). Just store the object directly.

Answer 2

I don't want to debate the usefulness of this utility class. I can provide some feedback which I think should allow you to make the code objectively more readable. Use C++14's additions to type_traits. That is you should e.g. use: std::enable_if_t<whatever> instead of typename std::enable_if<whatever>::type.
A question: is there any use case for explicitly specifying template parameter to get or get_mutable? If not, then you can omit adding the template parameter Another minor suggestion (that is my personal preference) is to use std::enable_if in template parameters specification, in my opinion this makes easier to quickly take note of the function signature. Furthermore, you could do something like:

 template<typename Q>
 using move_constructible = std::enable_if_t<std::is_move_constructible_v<Q>>;
 template<typename = move_constructible<_TLock<_TMtx>>>
 proxy<lock_type> get() {
 return proxy<lock_type>(obj_, lock_type(*mtx_));
 }
 template<typename = move_constructible<_TMutLock<_TMtx>>>
 proxy<mutable_lock_type> get_mutable() {
 return proxy<mutable_lock_type>(obj_, mutable_lock_type(mtx_));
 }

Of course if you don't feel like using C++17's std::is_move_constructible_v you can use C++11's std::is_move_constructible<>::value. If you want to preserve the ability for the caller to specify template arguments for those methods you can do this easily as well:

 template<typename Q = _TLock<_TMtx>, typename = move_constructible<Q>>
 proxy<lock_type> get() {
 return proxy<lock_type>(obj_, lock_type(*mtx_));
 } 

Same can be done for types that are not move constructible.
One more: most probably you should not use names starting with underscore followed by uppercase letter, see here.