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?

• Loki Astari: 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.
• Loki Astari: 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.
• icabod: @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".

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

• icabod: 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.
• Loki Astari: @icabod: Just change the internal representation to pointer. Maintain the interface that passes by reference.
• Loki Astari: @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()
• icabod: @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.
• ratchet freak: @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)