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?

Well, usually you use mutexes in an extremely careful way. When you lock a mutex, you want the lock to have a very short and carefully chosen duration so that you do not cause a deadlock. 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. Usually you never need to do it. — Chris Beck
Also if you specifically are passing "pthread_mutex &" to your mutex wrapper class, I mean that's kind of ugly... for one, you can get a dangling reference if you're not careful, and for two, you are then tied to the pthread implementation. Presumably the point of your class is to cover up the implementation details, but here you are blatantly leaking them! — Chris Beck
@ChrisBeck thanks for your attempt to explain. I request you to address the problem and its solution in an "answer". — Aquarius_Girl
@TheIndependentAquarius I started writing the answer before I saw these comments so there is already an answer that details the same points that ChrisBeck mentioned :) Maybe if Chris can edit the answer so it includes both of our contributions it will be a better answer! — Curious

Curious Curious · Accepted Answer · 2016-01-01T03:26:55

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.

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

2 Answers