Why is passing references to the Mutex class not a good design?

Question

From here: Logic error in my defined Mutex class and the way I use it in producer consumer program - pthreads

the way you pass around references(!) to your mutex class is plainly asking for trouble, it defies any kind of encapsulation.

Why is this a problem? Should I have passed by value and then written the copy constructor?

What harms can lack of encapsulation do in this case? How should I encapsulate what?

Also, Why is passing references to the Mutex class not a good design?

Passing a reference to the lock is a bad idea -- you don't "use" the lock, you only acquire and then you give it back. Moving it around makes it hard to track your use of the (critical) resource. Passing a reference to a mutex variable rather than a lock is maybe not so bad, but still it makes it harder to know what parts of the program may deadlock, so its something to avoid.

Please explain in simple language with examples - why is passing references a bad idea?

Answer 1

I'd distill this down to separate questions: 1. When is it appropriate to pass any object by reference? 2. When is it appropriate to share a mutex?

1. The way you pass an object as an argument reflects how you expect to share the lifetime of the object between the caller and the callee. If you pass by reference, you must assume either that the callee will be using the object only for duration of the call or, if the reference is stored by the callee, the callee's lifetime is shorter than the reference. If dealing with dynamically allocated objects, you should probably be using smart pointers, which (among other things) allow you to more explicitly communicate your intentions (see Herb Sutter's treatment of this subject).

2. Sharing a mutex should avoided. This is true whether passing by reference or any other means. By sharing a mutex, an object is allowing itself to be affected internally by an outside entity. That violates basic encapsulation and is reason enough. (See any text on object-oriented programming for the virtues of encapsulation). One real consequence in sharing the mutex is the likelihood of deadlock.

   Take this simple example:

   • A owns mutex
   • A shares mutex with B
   • B obtains the lock before calling function on A
   • A's function attempts to obtain the lock
   • Deadlock..

From a design perspective, why would you want to share a mutex? A mutex protects a resource that might be accessed by multiple threads. That mutex should be hidden (encapsulated) within a class that controls that resource. A mutex is just one way this class could protect the resource; its an implementation detail only the class should be aware of. Instead, share the instance of the class that controls the resource and allow it to ensure thread-safety within itself in whatever way it wants.

Answer 2

I think it's bad abstraction as well as bad encapsulation. a mutex is usually default constructed with copy constructors deleted, having multiple mutex objects that refer to the same logical object is error prone, i.e. it can lead to deadlocks and other race conditions because the programmer or the reader can assume that they are different entities.

Furthermore by specifying what internal mutex you are using you would be exposing the implementation details of your threads thereby breaking abstraction of the Mutex class. If you are using a pthread_mutex_t then you will most likely be using kernel threads (pthreads).

Encapsulation is also broken because your Mutex is not a single encapsulated entity but rather scattered into several (possibly dangling) references.

If you want to encapsulate a pthread_mutex_t into a class you would do it as so

class Mutex {
public:
    void lock(); 
    void unlock();

    // Default and move constructors are good! 
    // You can store a mutex in STL containers with these
    Mutex();
    Mutex(Mutex&&);
    ~Mutex();

    // These can lead to deadlocks!
    Mutex(const Mutex&) = delete;
    Mutex& operator= (const Mutex&) = delete;
    Mutex& operator= (Mutex&&) = delete;

private:
    pthread_mutex_t internal_mutex;
};

Mutex objects are meant to be shared in a shared scope declared in the implementation files rather than having it declared locally and passed around in functions as a reference. You would ideally only pass in arguments to the thread constructor that you need. Passing around references to objects declared in a scope at the same "level" as the function in question (thread execution in this case) usually leads to errors in code. What happens if the scope in which the mutex is declared is non existent anymore? Will the destructor of the mutex invalidate the inner implementation of the mutex? What happens if the mutex makes its way to a whole other module through being passed around and that module starts its own threads and thinks the mutex will never block, this can lead to nasty deadlocks.

Also one case where you would like to use the mutex move constructor is say in a mutex factory pattern, if you want to make a new mutex you would make a function call and that function would return a mutex which you would then add to your list of mutexes or pass to a thread that is requesting it through some sort of shared data (the aforementioned list would be a good idea for this shared data). Getting such a mutex factory pattern right however can be quite tricky as you need to lock access to the common list of mutexes. It should be a fun thing to try!

If the intention of the author was to avoid global scope then declaring it once in an implementation file as a static object should be enough abstraction.