Force acquiring R/W lock

Question

I am trying to build an abstraction that forces lock acquisition before data can be accessed.

This seems to work, but please tell me if what I am doing horribly wrong, and what I could improve.

I am using C++14 hence shared_timed_mutex

#include <mutex>
#include <shared_mutex>

template <class T>
class rw_locked
{
 public:
  class write_access
  {
   public:
    write_access(const write_access &) = delete;
    write_access &operator=(write_access &) = delete;
    write_access &operator=(const write_access &) = delete;
    write_access(write_access &&) = delete;
    write_access &operator=(write_access &&) = delete;

    write_access(rw_locked &t_locked)
        : m_lock(t_locked.m_mtx, std::defer_lock), m_hook(&t_locked.m_content){};

    T *operator->() { return m_hook; }
    T *get() { return m_hook; }

   private:
    T *m_hook;
    const std::shared_lock<std::shared_timed_mutex> m_lock;
  };

  class read_access
  {
   public:
    read_access(const read_access &) = delete;
    read_access &operator=(read_access &) = delete;
    read_access &operator=(const read_access &) = delete;
    read_access(read_access &&) = delete;
    read_access &operator=(read_access &&) = delete;

    read_access(rw_locked &t_locked)
        : m_lock(t_locked.m_mtx, std::defer_lock), m_hook(&t_locked.m_content){};

    const T *operator->() { return m_hook; }
    const T *get() { return m_hook; }


   private:
    const T *m_hook;
    const std::unique_lock<std::shared_timed_mutex> m_lock;
  };

  template <typename... content_args>
  explicit rw_locked(content_args &&...t_content_args)
      : m_mtx(), m_content(std::forward<content_args>(t_content_args)...)
  {
  }

 private:
  std::shared_timed_mutex m_mtx;
  T m_content;
};

It's meant to be used like this:

#include <iostream>
#include <string>
#include <vector>

//#include "rw_locked.h"

using locked_strings = rw_locked<std::vector<std::string>>;

int main()
{
  locked_strings strs;

  // these would be called from threads
  {
    locked_strings::write_access w_strs(strs);
    w_strs->push_back("1");
    w_strs->push_back("2");
    w_strs->push_back("3");
  }
  {
    locked_strings::read_access r_strs(strs);
    for (auto &it : *r_strs.get())
    {
      std::cout << it << "\n";
    }
  }
  return 0;
}

Thank you very much for your time!

Answer 1

The locks are never taken

By passing std::defer_lock when constructing m_lock, the mutexes will never be locked. Remove that parameter.

Swap std::shared_lock and std::unique_lock

Normally you want to allow multiple reads to share access to the data, but you only want to allow one unique writer at a time. So write_access should use a std::unique_lock, and read_access should use std::shared_lock.

Your example program is a very bad example; it never takes more than one lock at a time, so none of the properties you care about are tested. It shouldn't be difficult to add a multi-threaded example, see the example from std::shared_lock for how short it can be.

Add a test suite

The above issues could be found if you would add a test suite to your library that tests all the properties you want your classes to have.

Enable compiler warnings and fix them

GCC warns that m_lock and m_hook will be initialized in a different order than you specified in the constructors. Make sure you enable compiler warnings and fix all of them.

Add out-of-class helpers

Normally I am in favour of nesting classes where that makes sense. And in a way, it does so here as well. However, the problem is that now you have to remember the exact type. Having to write the following is very annoying:

rw_locked<std::vector<std::string>>::write_access w_strs(strs);

Which is why you used a using statement to reduce the amount you had to type. Consider how std::unique_lock is not a member of any mutex class. Wouldn't it be nice if you would have something similar, so you could write this instead?

write_access w_strs(strs);

I see two solutions. The first would be to move write_access and read_access out of rw_locked. They would become templates of their own of course, and it would require making them friends of rw_locked. It is possible, but a bit involved. Alternatively, keep the class hierarchy as it is, but add new helper classes that help deducing the right types. Consider:

template<typename RwLocked>
class write_access: public RwLocked::write_access
{
public:
    write_access(RwLocked &t_locked): RwLocked::write_access(t_locked) {}
};

Make it more like the STL's lock types

To make it even better, consider adopting the same naming scheme as the STL, and support all the things the STL does. For example, instead of read_access and write_access, call it shared_access and unique_access.

Furthermore, when creating a shared_access object for example, consider making it support everything that std::shared_lock does: support std::defer_lock_t and related tags, and since you are using a std::shared_timed_mutex, you want to support timeout_duration and timeout_time parameters as well. You could also support all the member function. In fact, you could consider just inheriting from std::shared_lock:

template<class T>
class rw_locked
{
public:
    using shared_lock_type = std::shared_lock<std::shared_timed_mutex>;

    class shared_access: public shared_lock_type
    {
    public:
        template<typename... lock_args>
        shared_access(rw_locked &t_locked, lock_args &&...t_lock_args)
            : shared_lock_type(t_locked.m_mtx,
                               std::forwward<lock_args>(t_lock_args)...)
            , m_hook(&t_locked.m_content) {}

        T *operator->() { return m_hook; }
        T *get() { return m_hook; }

    private:
        T* m_hook;
    }

    ...
};

But you could also argue that you should not do this; considering that the whole point is that access to the data is only possible if the lock is guaranteed to be held, and that the above would allow you to pass std::defer_lock_t or a duration that is too short while the lock was already taken by another thread, and the mutex might not be locked but you can still call get(). You could make get() and operator->() check that owns_lock() == true and throw some exception otherwise, but that adds a little bit of overhead. If you don't want that, you can still inherit from std::shared_lock like above, but then just never pass a tag, duration or time point, and then just use a std::shared_mutex instead of a std::shared_timed_mutex.