RAII-style lockable objects

Question

I am working on a Windows project using C++03 (C++11 is not an option), and without using Boost. I have several resources that are accessed by multiple threads at various times - a queue, for example, with data added by one thread and removed by another. Individual methods of the involved classes generally perform locking internally. However in some cases I need to lock the entire resource to perform a sequence of actions without another thread getting in the way.

Without the phrases "use Boost" and "use C++11", does the posted code for RAII-style locking of a resource look correct and safe, or is there something I've missed?

To this end I have written a simple RAII-style locking class, so that the following can be performed:

void somefunction(void) {
    GetLock<LockableClass> lock(queue);
    ...
}

Here the only requirement of LockableClass is that is has Lock() and Unlock() methods. Specialisations of the class are provided for CRITICAL_SECTION, etc.

However, in the case of there being several queues, only some of which need to be locked at one time, I would like the following to be possible:

void somefunction(void) {
    list<GetLock<LockableClass> > locks;
    for (vector<LockableClass>::iterator its = queues.begin(); queues.end() != its; ++its)
        if (some_condition)
            locks.push_back(GetLock<LockableClass>(*its));
    ...
}

In order to achieve this I've added a copy-constructor to GetLock. This creates the requirement that any lockable object has to have some kind of reference counting. It also limits me to using a std::list as the container for the locks - at least in my version of Visual Studio (2008 - yes, yes, I know). std::vector appears to default-construct, then assign.

The following is the code for GetLock, along with a specialisation for CRITICAL_SECTION:

template <typename TLOCKABLE>
class GetLock {
private:
    TLOCKABLE& lock;
    GetLock();
public:
    GetLock(TLOCKABLE& plock) : lock(plock) {
        lock.Lock();
    }
    GetLock(TLOCKABLE* plock) : lock(*plock) {
        lock.Lock();
    }
    GetLock(const GetLock<TLOCKABLE>& plock) : lock(plock.lock) {
        lock.Lock();
    }
    ~GetLock(void) {
        lock.Unlock();
    }
};

template <>
class GetLock<CRITICAL_SECTION> {
private:
    CRITICAL_SECTION& lock;
    GetLock();
public:
    GetLock(CRITICAL_SECTION& plock) : lock(plock) {
        EnterCriticalSection(&lock);
    }
    GetLock(const GetLock<CRITICAL_SECTION>& plock) : lock(plock.lock) {
        EnterCriticalSection(&lock);
    }
    ~GetLock(void) {
        LeaveCriticalSection(&lock);
    }
};

// There is also a specialisation for HANDLE, to cope with mutexes...
// This presents its own issues as HANDLE is used everywhere in Windows,
// but differentiating between a Mutex and something else is a question
// for StackOverflow.

Does the code look safe? Is there something I've missed? Is making the assumption of using recursively-lockable objects a foolhardy thing to do? What could I do better?

Answer 1

first I suggest you use pointers to keep the mutex reference; this makes it possible to have a non-owning state when the pointer is 0 (the state when default initialized)

also you created a copy constructor and a destructor, so you still need a copy assignment to properly follow rule of 3 (or at least disable it)

GetLock<TLOCKABLE>& operator=(const GetLock<TLOCKABLE>& plock) {
    if(plock.lock != lock){
        GetLock<TLOCKABLE> tmp(plock); 
        swap(tmp);

    }
    return *this;
}

however this keeps the assumption that the lock is recursively-lockable, but as an optimization it won't reacquire and release when the locks are the same