Thread-safe copy constructor/assignment operator

Let's say that we want to make class A thread-safe using an std::mutex. I am having my copy constructor and assignment operator similarly to the code below:

#include <mutex>

class A {
private:
  int i;
  mutable std::mutex mtx;

public:
  A() : i(), mtx() { }

  A(const A& other) : i(), mtx()
  {
    std::lock_guard<std::mutex> _lock(other.mtx);
    i = other.i;
  }

  A& operator=(const A& other)
  {
    if (this!=&other) {
      std::lock_guard<std::mutex> _mylock(mtx), _otherlock(other.mtx);
      i = other.i;
    }
    return *this;
  }

  int get() const
  {
    std::lock_guard<std::mutex> _mylock(mtx);
    return i;
  }
};

I do not think that it has any problems, other than the possibility of other to be destroyed by another thread before being copied, which I can deal with.

My issue is that I haven't seen this pattern anywhere, so I do not know if people just haven't had a need for that or that it is plainly wrong for reasons I currently don't see.

Thanks

NOTES:

This is just an example. I can have an arbitrary number of member variables of any type, it does not have to be just an int.

After Martin York's comment for possible deadlocking, this is an updated version that uses copy-and-swap (copy elision is also possible, but it wouldn't handle efficiently开发者_开发技巧 the self-assignment case).

I also changed int to T, so people cannot assume that it is a POD.

template<typename T>
class A {
private:
  T t;
  mutable std::mutex mtx;

public:
  A() : t(), mtx() { }

  A(const A& other) : t(), mtx()
  {
    std::lock_guard<std::mutex> _lock(other.mtx);
    t = other.t;
  }

  A& operator=(const A& other)
  {
    if (this!=&other) {
      A tmp(other);
      std::lock_guard<std::mutex> _lock(mtx);
      std::swap(t, tmp.t);
    }
    return *this;
  }

  T get() const
  {
    std::lock_guard<std::mutex> _lock(mtx);
    return t;
  }

};

Old question, new answer:

Imho, a better way to deal with the dead-lock problem of the original copy assignment operator is:

  A& operator=(const A& other)
  {
    if (this!=&other) {
      std::unique_lock<std::mutex> _mylock(mtx, std::defer_lock),
                                   _otherlock(other.mtx, std::defer_lock);
      std::lock(_mylock, _otherlock);
      i = other.i;
    }
    return *this;
  }

I.e. use std::lock(L1, L2) to simultaneously lock the two mutexes without fear of deadlock. This is likely to be higher performance than the copy/swap idiom, especially if the member data consists of std::vector, std::string, or types that contain vectors and/or strings.

In C++1y (we hope y is 4), there is a new <shared_mutex> header providing read/write lock capability which may provide a performance boost (performance testing would be necessary for specific use cases to confirm that). Here is how it would be used:

#include <mutex>
#include <shared_mutex>

class A {
private:
  int i;
  mutable std::shared_mutex mtx;

public:
  A() : i(), mtx() { }

  A(const A& other) : i(), mtx()
  {
    std::shared_lock<std::shared_mutex> _lock(other.mtx);
    i = other.i;
  }

  A& operator=(const A& other)
  {
    if (this!=&other) {
      std::unique_lock<std::shared_mutex> _mylock(mtx, std::defer_lock);
      std::shared_lock<std::shared_mutex> _otherlock(other.mtx, std::defer_lock);
      std::lock(_mylock, _otherlock);
      i = other.i;
    }
    return *this;
  }

  int get() const
  {
    std::shared_lock<std::shared_mutex> _mylock(mtx);
    return i;
  }
};

I.e. this is very similar to the original code (modified to use std::lock as I've done above). But the member mutex type is now std::shared_mutex instead of std::mutex. And when protecting a const A (and assuming no mutable members besides the mutex), one need only lock the mutex in "shared mode". This is easily done using shared_lock<shared_mutex>. When you need to lock the mutex in "exclusive mode", you can use unique_lock<shared_mutex>, or lock_guard<shared_mutex> as appropriate, and in the same manner as you would have used this facilities with std::mutex.

In particular note that now many threads can simultaneously copy construct from the same A, or even copy assign from the same A. But only one thread can still copy assign to the same A at a time.

Ignoring all implementation details, the reason you don't see this pattern is because it is very likely that you are locking on the wrong abstraction level.

• If the objects are accessed from multiple threads, then you (additionally) have to manage the lifetime of the objects, which cannot be managed from within the objects.
• For lifetime management you already need at least one object-external lock, so better use that.
• This scheme only makes sense for single-get() objects -- if your object has more (than one member and more) than one get() function, then reading from the object can/will result in inconsistent data.

Getting correct multithreaded code isn't just a matter of makeing sure nothing "crashes" and single objects stay in a consistent state. And if you (think you) need the above scheme you may think you're save when your app is still doing-the-wrong-thing.

As for implementation details: Since you are already using C++0x for this, you also should implement appropriately defined move operations.

I'm not an authority on this, because multi-threading is tricky, but it looks fine so far. BTW, you probably meant

std::lock_guard<std::mutex>

and in the copy-ctor:

A(const A& other) : mtx()
{
  std::lock_guard<std::mutex> _lock(other.mtx);
  i = other.i;
}

Another way to ensure thread-safety for other is only using 'safe' getters to access it, although this would not behave as expected when multiple getters are called. But, beware of references!

You don't really see it because the Standard threading facilities are exceedingly new and I don't know of a single compiler that supports them - you'd get further looking for boost::thread examples. Also, your gratuitous use of synchronization will likely lead to poor performance, but that's just my opinion.

this is more correct, but not entirely robust:

#include <mutex>

class A {
private:
    int i;
    std::mutex mtx;

public:
    A() : i(0), mtx() {
    }
    /* this is one option for implementation, but would be rewritten when there are more ivars in order to reduce acquisition counts */
    A(A& other) : i(other.get()), mtx() {
    }

    ~A() {
        /* unsafe if subclassed, also useful for determining whether objects are destroyed prematurely (e.g., by their containers */
        std::lock_guard<std::mutex> _mylock(this->mtx);
    }

    A& operator=(A& other) {
        std::lock_guard<std::mutex> _mylock(this->mtx);
        std::lock_guard<std::mutex> _otherlock(other.mtx);
        this->i = other.i; /* you could call other.get() and bypass the lock_guard, but i'm assuming there's really more work to be done here */
        return *this;
    }

    int get() {
        std::lock_guard<std::mutex> _mylock(this->mtx);
        return this->i;
    }
private:
    /* prohibited */
    A(const A& other);
    /* also prohibited */
    A& operator=(const A& other);
};