How should I deal with mutexes in movable types in C++?

Let’s start with a bit of code:

class A
{
    using MutexType = std::mutex;
    using ReadLock = std::unique_lock<MutexType>;
    using WriteLock = std::unique_lock<MutexType>;

    mutable MutexType mut_;

    std::string field1_;
    std::string field2_;

public:
    ...

I’ve put some rather suggestive type aliases in there that we won’t really take advantage of in C++11, but become much more useful in C++14. Be patient, we’ll get there.

Your question boils down to:

How do I write the move constructor and move assignment operator for this class?

We’ll start with the move constructor.

Move Constructor

Note that the member mutex has been made mutable. Strictly speaking this isn’t necessary for the move members, but I’m assuming you also want copy members. If that is not the case, there is no need to make the mutex mutable.

When constructing A, you do not need to lock this->mut_. But you do need to lock the mut_ of the object you’re constructing from (move or copy). This can be done like so:

    A(A&& a)
    {
        WriteLock rhs_lk(a.mut_);
        field1_ = std::move(a.field1_);
        field2_ = std::move(a.field2_);
    }

Note that we had to default construct the members of this first, and then assign them values only after a.mut_ is locked.

Move Assignment

The move assignment operator is substantially more complicated because you do not know if some other thread is accessing either the lhs or rhs of the assignment expression. And in general, you need to guard against the following scenario:

// Thread 1
x = std::move(y);

// Thread 2
y = std::move(x);

Here is the move assignment operator that correctly guards the above scenario:

    A& operator=(A&& a)
    {
        if (this != &a)
        {
            WriteLock lhs_lk(mut_, std::defer_lock);
            WriteLock rhs_lk(a.mut_, std::defer_lock);
            std::lock(lhs_lk, rhs_lk);
            field1_ = std::move(a.field1_);
            field2_ = std::move(a.field2_);
        }
        return *this;
    }

Note that one must use std::lock(m1, m2) to lock the two mutexes, instead of just locking them one after the other. If you lock them one after the other, then when two threads assign two objects in opposite order as shown above, you can get a deadlock. The point of std::lock is to avoid that deadlock.

Copy Constructor

You didn’t ask about the copy members, but we might as well talk about them now (if not you, somebody will need them).

    A(const A& a)
    {
        ReadLock  rhs_lk(a.mut_);
        field1_ = a.field1_;
        field2_ = a.field2_;
    }

The copy constructor looks much like the move constructor except the ReadLock alias is used instead of the WriteLock. Currently these both alias std::unique_lock<std::mutex> and so it doesn’t really make any difference.

But in C++14, you will have the option of saying this:

    using MutexType = std::shared_timed_mutex;
    using ReadLock  = std::shared_lock<MutexType>;
    using WriteLock = std::unique_lock<MutexType>;

This may be an optimization, but not definitely. You will have to measure to determine if it is. But with this change, one can copy construct from the same rhs in multiple threads simultaneously. The C++11 solution forces you to make such threads sequential, even though the rhs isn’t being modified.

Copy Assignment

For completeness, here is the copy assignment operator, which should be fairly self explanatory after reading about everything else:

    A& operator=(const A& a)
    {
        if (this != &a)
        {
            WriteLock lhs_lk(mut_, std::defer_lock);
            ReadLock  rhs_lk(a.mut_, std::defer_lock);
            std::lock(lhs_lk, rhs_lk);
            field1_ = a.field1_;
            field2_ = a.field2_;
        }
        return *this;
    }

And etc.

Any other members or free functions that access A‘s state will also need to be protected if you expect multiple threads to be able to call them at once. For example, here’s swap:

    friend void swap(A& x, A& y)
    {
        if (&x != &y)
        {
            WriteLock lhs_lk(x.mut_, std::defer_lock);
            WriteLock rhs_lk(y.mut_, std::defer_lock);
            std::lock(lhs_lk, rhs_lk);
            using std::swap;
            swap(x.field1_, y.field1_);
            swap(x.field2_, y.field2_);
        }
    }

Note that if you just depend on std::swap doing the job, the locking will be at the wrong granularity, locking and unlocking between the three moves that std::swap would internally perform.

Indeed, thinking about swap can give you insight into the API you might need to provide for a “thread-safe” A, which in general will be different from a “non-thread-safe” API, because of the “locking granularity” issue.

Also note the need to protect against “self-swap”. “self-swap” should be a no-op. Without the self-check one would recursively lock the same mutex. This could also be solved without the self-check by using std::recursive_mutex for MutexType.

Update

In the comments below Yakk is pretty unhappy about having to default construct things in the copy and move constructors (and he has a point). Should you feel strongly enough about this issue, so much so that you are willing to spend memory on it, you can avoid it like so:

• Add whatever lock types you need as data members. These members must come before the data that is being protected:

  mutable MutexType mut_;
  ReadLock  read_lock_;
  WriteLock write_lock_;
  // ... other data members ...
  

• And then in the constructors (e.g. the copy constructor) do this:

  A(const A& a)
      : read_lock_(a.mut_)
      , field1_(a.field1_)
      , field2_(a.field2_)
  {
      read_lock_.unlock();
  }
  

Oops, Yakk erased his comment before I had the chance to complete this update. But he deserves credit for pushing this issue, and getting a solution into this answer.

Update 2

And dyp came up with this good suggestion:

    A(const A& a)
        : A(a, ReadLock(a.mut_))
    {}
private:
    A(const A& a, ReadLock rhs_lk)
        : field1_(a.field1_)
        , field2_(a.field2_)
    {}