Mutex locker class in C++

Question

What needs to be corrected, added, or subtracted here?

class mutexLocker
{
    private:
    /* Declaration of a Mutex variable `mutexA`. */
    pthread_mutex_t &mutexA;

    /* `mutexStatus` holds the return value of the function`pthread_mutex_lock `. 
    This value has to be returned to the callee so we need to preserve it in a class
    variable. */
    int             mutexStatus;

    public:
    /* Constructor attempts to lock the desired mutex variable. */
    mutexLocker (pthread_mutex_t argMutexVariable) 
    : mutexA (argMutexVariable)
    {
        /* Return value is needed in order to know whether the mutex has been 
        successfully locked or not. */
        int mutexStatus = pthread_mutex_lock (&argMutexVariable);
    }

    /* Since the constructor can't return anything, we need to have a separate function
    which returns the status of the lock. */
    int getMutexLockStatus ()
    {
        return mutexStatus;
    }

    /* We may need this Mutex variable as an argument for `pthread_cond_wait()`*/
    pthread_mutex_t getLockedMutex ()
    {
        if (mutexStatus >= 0)
            return mutexA;
    }

    /* The destructor will get called automatically whereever the callee's scope ends, and
    will get the mutex unlocked. */
    ~mutexLocker ()
    {
        if (mutexStatus >= 0)  
             pthread_mutex_unlock (&mutexA);
    }
};

Answer 1

If you are keeping a reference to an object you should pass that object to the constructor as a reference:

mutexLocker (pthread_mutex_t    argMutexVariable) 
                    //     ^^^  Missing reference.

You return the state of the lock operation:

int getMutexLockStatus ()

But if the mutext failed to lock what are you going to do.
If the mutext fails to lock there is probably something fundamentally wrong so you should probably stop the current action by throwing an exception.

You can't conditionally return a value.
You must always return something from a function that expects to return value.

pthread_mutex_t getLockedMutex ()
{
    if (mutexStatus >= 0)
        return mutexA;


    // What happens if the code gets here?
    // Letting a function return without an explicit return (in a non void method)
    // is undefined behavior.
}

Also I see no point in testing the status.
If the lock failed to lock you should have exited the code much earlier than this. If the lock fails throw an exception.

Your test is also incorrect (here and the destructor)

if (mutexStatus >= 0)  

If the mutex did lock then it returned 0 and thus the mutexStatus is zero.

Answer 2

Think about it this way: The sole purpose of the locker class is to provide a scoped variable that ex­pres­ses the notion of the associated mutex being locked. The mere existence of a locker variable is sup­pos­ed to guarantee that you are the sole execution inside a critical section.

Thus it makes no sense for your class to have an error state. If you cannot successfully obtain a lock, you should throw an exception, since it is not possible for your object to be constructed if there is an error.

Moreover, you have to take the mutex variable by reference.

Putting it all together, the following picture emerges:

struct MutexLocker
{
    MutexLocker(pthread_mutex_t & m)
    : _m(m)
    {
        if (pthread_mutex_lock(&_m) != 0)
        {
            throw std::logic_error("Could not lock mutex");
        }
    }

    ~MutexLocker()
    {
        pthread_mutex_unlock(&m);
    }

private:
    pthread_lock_t & _m;
};

If you check the documentation for pthread_mutex_unlock, you will find that our destructor is indeed correct and can never fail.

---

If you do want to have a C++ wrapper around a mutex that allows trivial construction, optional locking and early unlocking, you will need to write a more comprehensive, complex class. I believe std::unique_lock is a standard library template that already possesses such a set of features; you should take a look at what it offers.