2
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – Martin York Nov 15 '13 at 14:31
  • \$\begingroup\$ 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. \$\endgroup\$ – Martin York Nov 15 '13 at 17:24
  • \$\begingroup\$ @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". \$\endgroup\$ – icabod Nov 19 '13 at 10:08
3
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – icabod Nov 15 '13 at 13:08
  • \$\begingroup\$ @icabod: Just change the internal representation to pointer. Maintain the interface that passes by reference. \$\endgroup\$ – Martin York Nov 15 '13 at 14:28
  • \$\begingroup\$ @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() \$\endgroup\$ – Martin York Nov 15 '13 at 17:19
  • \$\begingroup\$ @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. \$\endgroup\$ – icabod Nov 18 '13 at 10:13
  • \$\begingroup\$ @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) \$\endgroup\$ – ratchet freak Nov 18 '13 at 10:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.