How to Deal With Mutexes in Movable Types in C++

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_)
    {}

How to copy/move a class containing a mutex

In your implementation of operator=, you need to specify the class name:

Carrier& Carrier::operator = (const Carrier& other).

You're getting the compilation error you described when locking the mutex because std::mutex::lock() is not a const method. In the assignment operator, you take the other parameter by const reference, as you should. Thus, it's trying to call a non-const function on a const object, causing the compilation error.

The workaround is to declare the member as mutable in the declaration of the Carrier class:

mutable std::mutex safeRead;

This allows you to work around the compile error.

Why is std::mutex neither copyable nor movable?

Expanding a bit on the other answers, basic locks such as Mutexes are the most basic objects in the language design providing atomic operations, lock and unlock here. These might own an OS implemented handle (native_handle) that is a handle for a hardware implemented object, and might even skip the intermediate handle.

Copying such a handle of course, is non-trivial (you can't copy a piece of hardware, and sometimes even a OS handle, trivially). Moving it is potentially worse - move leaves the object in an unspecified state, but by its nature, a mutex is shared across threads. If you gut it on one thread, you would somehow have to inform all other threads - more likely you would just have breaking code. This is a lot of overhead for no potential benefit (I can see).

As to why the move constructor is not explicitly deleted in your reference - no default move constructor is created if there is a (non-default) defined destructor (12.8, comment 9), so there is no need to delete it.

Move constructor for std::mutex

Well... mainly because I don't think they should move. Literally.

In some OS-es a mutex might be modeled as a handle (so you could copy them) but IIRC a pthreads mutex is manipulated in-place. If you are going to relocate that, any threadsafety is going fly right out of the window (how would the other threads know that the mutex had just changed it's memory address...)?

Does move constructor makes any sense for a class that has std::thread's and std::mutex's as its fields?

Let's break the issue in two.

Moving mutexes. This cannot be done because mutexes are normally implemented in terms of OS objects which must have fixed addresses. In other words, the OS (or the runtime library, which is the same as the OS for our purposes) keeps an address of your mutex. This can be worked around by storing (smart) pointers to mutexes in your code, and moving those instead. The mutex itself doesn't move. Thread objects can be moved so there's no issue.
Moving your own data whereas some active code (a thread or a running function or a std::function stored somewhere or whatever) has the address of your data and can access it. This is actually very similar to the previous case, only instead of the OS it's your own code that holds on the data. The solution, as before, is in not moving your data. Store and move a (smart) pointer to the data instead.

To summarise,

class WonnaBeMovedClass
{
public:
    WonnaBeMovedClass
        (WonnaBeMovedClass&& other);
    void start();
private:
    struct tdata {
        std::vector<int> _sharedData;
        std::thread _threadUpdate;
        std::mutex _mutexShaderData;
     };
     std::shared_ptr<tdata> data;
     static void updateSharedData(std::shared_ptr<tdata>);
};

void WonnaBeMovedClass::start()
{
    _threadUpdate = std::thread(&updateSharedData, data);
}

Mutex as object attribute and vector of objects

As the error message explains, you need to provide a copy-assignment operator, e.g.:

Shelf& operator= (const Shelf &a)
{
    if (this != &a)
    {
        WriteLock lhs_lk (m_mutex, std::defer_lock);
        ReadLock rhs_lk (a.m_mutex, std::defer_lock);
        std::lock (lhs_lk, rhs_lk);

        m_genre = a.m_genre;
        m_shelf = a.m_shelf;
    }
    return *this;
}

The presence of either a user-defined move constructor or a user-defined move assignment operator makes this necessary. You have both.

Where should the condition variables, mutexes be declared when the threads deal with different classes?

Normally we declare condition variables, mutexes in the same class because we use both for controlling, scheduling and prioritize threads. I don't think we need to declare globally for condition and mutexes. But still if you really need that you can use a namespace here.

Boost interprocess_mutex copy/move constructor?

I solved the problem by changing the vector to contain shared_ptr instead of test directly. test() gets called only once and the object is owned by the shared_ptr object, which is move/copyable:

    using namespace boost::interprocess;

    typedef managed_shared_memory::segment_manager SegmentManager;
    typedef allocator<void, SegmentManager> test_allocator;
    typedef deleter<test, SegmentManager> test_deleter;
    typedef shared_ptr<test, test_allocator, test_deleter> test_pointer;
    typedef vector<test_pointer, allocator<test_pointer, SegmentManager>> test_pointer_vector;

    managed_shared_memory seg(open_or_create, "MySharedMemory", 65536);

    test_allocator alloc(seg.get_segment_manager());
    test_deleter del(seg.get_segment_manager());

    test_pointer& p = *seg.construct<test_pointer>(anonymous_instance)(seg.construct<test>(anonymous_instance)(), alloc, del);
    test_pointer_vector& vec = *seg.construct<test_pointer_vector>(anonymous_instance)(alloc);

    vec.push_back(p);
    p.get()->mutex_.try_lock();