# RAII-style lockable objects

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?

• std::vector appears to default-construct, then assign. No It copy constructs into place. Because you have already created the object before the push_back is called. – Martin York Nov 15 '13 at 14:31
• Usually when you have multiple lockable objects. You want to acquire them in a specific order (order to avoid deadlock). TO do this you must also release locks that you previously held if they are not acquired in the correct order. – Martin York Nov 15 '13 at 17:24
• @LokiAstari: You're right regarding the push_back, but it seems that std::vector requires op= to be declared too, which is where my original code failed. I've updated my question with the "final" code, which I think works quite well. And yes, normally you may need to lock in a specific order, but for my purpose it's not so necessary... just that they all be locked prior to "some process". – icabod Nov 19 '13 at 10:08

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

• Ah yes, I forgot about at least declaring operator=. And I'm not sure why I went with a reference, but changing to a pointer would allow your code above, and hence allow putting locks into a vector (if I allow default construction). Good stuff, thanks. – icabod Nov 15 '13 at 13:08
• @icabod: Just change the internal representation to pointer. Maintain the interface that passes by reference. – Martin York Nov 15 '13 at 14:28
• @ratchet You should use copy and swap idium for assignment operator. Its exception safe while this is not. If lock->Lock() throws then you fail to call tmp->Unlock() – Martin York Nov 15 '13 at 17:19
• @LokiAstari: Good call on the copy-swap... the whole point of this class is to be exception safe. If I'm swapping to a pointer to the lockable object, then it makes copy-swap simple. Thanks. – icabod Nov 18 '13 at 10:13
• @icabod yeah then the entire if block will be just GetLock<TLOCKABLE> tmp(plock); swap(tmp); with swap just swapping the lock pointers (which is exception safe) – ratchet freak Nov 18 '13 at 10:19