Thread-safe copy constructor/assignment operator

Question

Hi,

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;
  }

};

Answer 1

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!

Answer 2

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.

Answer 3

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);
};

Answer 4

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.