Designing a thread-safe copyable class

Question

The straightforward way to make a class threadsafe is to add a mutex attribute and lock the mutex in the accessor methods

class cMyClass {
  boost::mutex myMutex;
  cSomeClass A;
public:
  cSomeClass getA() {
    boost::mutex::scoped_lock lock( myMutex );
    return A;
  }
};

The problem is that this makes the class non-copyable.

I can make things work by making the mutex a static. However, this means that every instance of the class blocks when any other instance is being accessed, because they all share the same mutex.

I wonder if there is a better way?

My conclusion is that there is no better way. Making a class thread-safe with private static mutex attribute is ‘best’: - it is simple, it works, and it hides the awkward details.

class cMyClass {
  static boost::mutex myMutex;
  cSomeClass A;
public:
  cSomeClass getA() {
    boost::mutex::scoped_lock lock( myMutex );
    return A;
  }
};

The disadvantage is that all instances of the class share the same mutex and so block each other unnecessarily. This cannot be cured by making the mutex attribute non-static ( so giving each instance its own mutex ) because the complexities of copying and assignment are nightmarish, if done properly.

The individual mutexes, if required, must be managed by an external non-copyable singleton with links established to each instance when created.

---

Thanks for all the responses.

Several people have mentioned writing my own copy constructor and assignment operator. I tried this. The problem is that my real class has many attributes which are always changing during development. Maintaining both the copy constructor and assignmet operator is tedious and error-prone, with errors creating hard to find bugs. Letting the compiler generate these for complex class is an enormous time saver and bug reducer.

---

Many responses are concerned about making the copy constructor and assignment operator thread-safe. This requirement adds even more complexity to the whole thing! Luckily for me, I do not need it since all the copying is done during set-up in a single thread.

---

I now think that the best approach would be to build a tiny class to hold just a mutex and the critical attributes. Then I can write a small copy constructor and assignment operator for the critical class and leave the compiler to look after all the other attributes in the main class.

class cSafe {
  boost::mutex myMutex;
  cSomeClass A;
public:
  cSomeClass getA() {
    boost::mutex::scoped_lock lock( myMutex );
    return A;
  }
  (copy constructor)
  (assignment op )

};
class cMyClass {
  cSafe S;
  ( ... other attributes ... )
public:
  cSomeClass getA() {
    return S.getA();
  }
};

Answer 1

You can define your own copy constructor (and copy assignment operator). The copy constructor would probably look something like this:

cMyClass(const cMyClass& x) : A(x.getA()) { }

Note that getA() would need to be const-qualified for this to work, which means the mutex would need to be mutable; you could make the parameter a non-const reference, but then you can't copy temporary objects, which usually isn't desirable.

Also, consider that it isn't always a good idea to perform locking at such a low level: if you lock the mutex in the accessor and the mutator functions, you lose a lot of functionality. For example, you can't perform a compare-and-swap because you can't get and set the member variable with a single lock of the mutex, and if you have multiple data members controlled by the mutex, you can't access more than one of them with the mutex locked.