Is this an acceptable way to lock a container using C++?

Question

I need to implement (in C++) a thread safe container in such a way that only one thread is ever able to add or remove items from the container. I have done this kind of thing before by sharing a mutex between threads. This leads to a lot of mutex objects being littered throughout my code and makes things very messy and hard to maintain.

I was wondering if there is a neater and more object oriented way to do this. I thought of the following simple class wrapper around the container (semi-pseudo C++ code)

 class LockedList {
    private:
        std::list<MyClass> m_List;

    public:
        MutexObject Mutex;
 };

so that locking could be done in the following way

 LockedList lockableList;     //create instance
 lockableList.Mutex.Lock();    // Lock object

 ... // search and add or remove items

 lockableList.Mutex.Unlock();   // Unlock object

So my question really is to ask if this is a good approach from a design perspective? I know that allowing public access to members is frowned upon from a design perspective, does the above design have any serious flaws in it. If so is there a better way to implement thread safe container objects?

I have read a lot of books on design and C++ in general but there really does seem to be a shortage of literature regarding multithreaded programming and multithreaded software design.

If the above is a poor approach to solving the problem I have could anyone suggest a way to improve it, or point me towards some information that explains good ways to design classes to be thread safe??? Many thanks.

Answer 1

I would rather design a resourece owner that locks a mutex and returns an object that can be used by the thread. Once the thread has finished with it and stops using the object the resource is automatically returned to its owner and the lock released.

template<typename Resource>
class ResourceOwner
{
      Lock         lock; 
      Resource     resource;

      public:
         ResourceHolder<Resource>  getExclusiveAccess()
         {
              // Let the ResourceHolder lock and unlock the lock
              // So while a thread holds a copy of this object only it
              // can access the resource. Once the thread releases all
              // copies then the lock is released allowing another
              // thread to call getExclusiveAccess().
              //
              // Make it behave like a form of smart pointer
              //    1) So you can pass it around.
              //    2) So all properties of the resource are provided via ->
              //    3) So the lock is automatically released when the thread
              //       releases the object.

              return ResourceHolder<Resource>(lock, resource);
         }
};

The resource holder (not thought hard so this can be improved)

template<typename Resource>
class ResourceHolder<
{
    // Use a shared_ptr to hold the scopped lock
    // When first created will lock the lock. When the shared_ptr
    // destroyes the scopped lock (after all copies are gone)
    // this will unlock the lock thus allowding other to use
    // getExclusiveAccess() on the owner
    std::shared_ptr<scopped_lock>    locker;
    Resource&                        resource;   // local reference on the resource.

    public:
        ResourceHolder(Lock& lock, Resource& r)
            : locker(new scopped_lock(lock))
            , resource(r)
        {}

        // Access to the resource via the -> operator
        // Thus allowing you to use all normal functionality of 
        // the resource.
        Resource* operator->() {return &resource;}
};

Now a lockable list is:

ResourceOwner<list<int>>  lockedList;

void threadedCode()
{
    ResourceHolder<list<int>>  list = lockedList.getExclusiveAccess();

    list->push_back(1);
}
// When list goes out of scope here. 
// It is destroyed and the the member locker will unlock `lock`
// in its destructor thus allowing the next thread to call getExclusiveAccess()

Answer 2

I would do something like this to make it more exception-safe by using RAII.

class LockedList {
private:
    std::list<MyClass> m_List;
    MutexObject Mutex;
    friend class LockableListLock;
};

class LockableListLock {
private:
    LockedList& list_;
public:
    LockableListLock(LockedList& list) : list_(list) { list.Mutex.Lock(); }
    ~LockableListLock(){ list.Mutex.Unlock(); }
}

You would use it like this

LockableList list;
{
    LockableListLock lock(list); // The list is now locked.

    // do stuff to the list

} // The list is automatically unlocked when lock goes out of scope.

You could also make the class force you to lock it before doing anything with it by adding wrappers around the interface for std::list in LockableListLock so instead of accessing the list through the LockedList class, you would access the list through the LockableListLock class. For instance, you would make this wrapper around std::list::begin()

std::list::iterator LockableListLock::begin() {
    return list_.m_List.begin();
}

and then use it like this

LockableList list;
LockableListLock lock(list);
// list.begin();   //This is a compiler error so you can't 
                   //access the list without locking it
lock.begin(); // This gets you the beginning of the list

Answer 3

Okay, I'll state a little more directly what others have already implied: at least part, and quite possibly all, of this design is probably not what you want. At the very least, you want RAII-style locking.

I'd also make the locked (or whatever you prefer to call it) a template, so you can decouple the locking from the container itself.

// C++ like pesudo-code. Not intended to compile as-is.
struct mutex {
    void lock() { /* ... */ }
    void unlock() { /* ... */ }
};

struct lock {
    lock(mutex &m) { m.lock(); }
    ~lock(mutex &m) { m.unlock(); }
};

template <class container>
class locked {
    typedef container::value_type value_type;
    typedef container::reference_type reference_type;
    // ...

    container c;
    mutex m;
public:
    void push_back(reference_type const t) {
        lock l(m);
        c.push_back(t);
    }

    void push_front(reference_type const t) { 
        lock l(m);
        c.push_front(t);
    }

    // etc.
};

This makes the code fairly easy to write and (for at least some cases) still get correct behavior -- e.g., where your single-threaded code might look like:

std::vector<int> x;

x.push_back(y);

...your thread-safe code would look like:

locked<std::vector<int> > x;

x.push_back(y);

Assuming you provide the usual begin(), end(), push_front, push_back, etc., your locked<container> will still be usable like a normal container, so it works with standard algorithms, iterators, etc.

Answer 4

The problem with this approach is that it makes LockedList non-copyable. For details on this snag, please look at this question:

Designing a thread-safe copyable class

I have tried various things over the years, and a mutex declared beside the the container declaration always turns out to be the simplest way to go ( once all the bugs have been fixed after naively implementing other methods ).

You do not need to 'litter' your code with mutexes. You just need one mutex, declared beside the container it guards.