In C++11, is it wise (or even safe) to use std::unique_lock<std::mutex> as a class member? If so, are there any guidelines?

Question

Is it wise (or even safe) to use std::unique_lock as a class member? If so, are there any guidelines?

My thinking in using std::unique_lock was to ensure that the mutex is unlocked in the case of an exception being thrown.

The following code gives an example of how I'm currently using the unique_lock. I would like to know if I'm going in the wrong direction or not before the project grows too much.

#include <iostream>
#include <string>
#include <thread>
#include <mutex>
#include <unistd.h>


class WorkerClass {
private:
    std::thread workerThread;
    bool workerThreadRunning;
    int workerThreadInterval;

    int sharedResource;

    std::mutex mutex;
    std::unique_lock<std::mutex> workerMutex;

public:
    WorkerClass() {
        workerThreadRunning = false;
        workerThreadInterval = 2;

        sharedResource = 0;

        workerMutex = std::unique_lock<std::mutex>(mutex);

        unlockMutex();
    }


    ~WorkerClass() {
        stopWork();
    }


    void startWork() {
        workerThreadRunning = true;
        workerThread = std::thread(&WorkerClass::workerThreadMethod,
                                   this);
    }


    void stopWork() {
        lockMutex();
        if (workerThreadRunning) {
            workerThreadRunning = false;
            unlockMutex();
            workerThread.join();
        }else {
            unlockMutex();
        }
    }


    void lockMutex() {
        try {
            workerMutex.lock();
        }catch (std::system_error &error) {
            std::cout << "Already locked" << std::endl;
        }
    }


    void unlockMutex() {
        try {
            workerMutex.unlock();
        }catch (std::system_error &error) {
            std::cout << "Already unlocked" << std::endl;
        }
    }

    int getSharedResource() {
        int result;
        lockMutex();
        result = sharedResource;
        unlockMutex();
        return result;
    }


    void workerThreadMethod() {
        bool isRunning = true;

        while (isRunning) {
            lockMutex();
            sharedResource++;
            std::cout << "WorkerThread:  sharedResource = "
                      << sharedResource << std::endl;
            isRunning = workerThreadRunning;
            unlockMutex();

            sleep(workerThreadInterval);
        }
    }
};



int main(int argc, char *argv[]) {
    int sharedResource;
    WorkerClass *worker = new WorkerClass();

    std::cout << "ThisThread: Starting work..." << std::endl;
    worker->startWork();

    for (int i = 0; i < 10; i++) {
        sleep(1);

        sharedResource = worker->getSharedResource();
        std::cout << "ThisThread: sharedResource = "
                  << sharedResource << std::endl;
    }

    worker->stopWork();

    std::cout << "Done..." << std::endl;

    return 0;
}

Answer 1

this is actually quite bad. storing a std::unique_lock or std::lock_guard as a member variable misses the point of scoped locking, and locking in general.

the idea is to have shared lock between threads, but each one temporary locks the shared resource the lock protects. the wrapper object makes it return-from-function safe and exception-safe.

you first should think about your shared resource. in the context of "Worker" I'd imagine some task queue. then, that task queue is associated with a some lock. each worker locks that lock with scoped-wrapper for queuing a task or dequeuing it. there is no real reason to keep the lock locked as long as some instance of a worker thread is alive, it should lock it when it needs to.