sharing a data member with the outside world in a thread-safe manner

Question

I would really appreciate some advise on this matter.

e.g.

class Foo
{
    TData data;
public:
    TData *getData() { return &data; } // How can we do this in a thread safe manner ?
};

So I want to have a mechanism to make getData() thread-safe. I have come up with my own solution which involves packing the data member in the following template class with a mutex used to synchronize access to it. What do you think ? What might be the possible problems ?

class locked_object : boost::noncopyable
{
    T *object;
    TLockable *lock;
    bool locked;

public:
    locked_object(T *_object, TLockable *_lock) : object(_object), lock(_lock), locked(true)
    {
        lock->lock();
    }

    ~locked_object()
    {
        lock->unlock();
    }

    T *get()
    {
        _ASSERT(locked);
        if (!locked)
            throw new std::exception("Synchronization error ! Object lock is already released !");
        return this->tobject;
    }


    void unlock()
    {
        locked = false;

        lock->unlock();
    }

    T *operator ->() const
    {   
        _ASSERT(locked);
        if (!locked)
            throw new std::exception("Synchronization error ! Object lock is already released !");

        return this->tobject; 
    }

    operator T *() const
    {
        _ASSERT(locked);
        if (!locked)
            throw new std::exception("Synchronization error ! Object lock is already released !");

        return this->tobject;
    }
};

Thank you for any comments and opinions in advance.

Fatih

Answer 1

Have you ever heard of The Law of Demeter ?

There is a similar advise (from Sutter I think): Don't share references to your internals

Both are meant to avoid coupling, because by sharing a reference to your internal, it means that your public interface leaks an implementation detail.

Now that this is said, your interface does not work.

The problem is that your are locking the proxy, not the object: I can still access through multiple paths:

• from Foo, no mutex required --> oups ?
• from two different locked_object --> this doesn't seem intentional...

More importantly, you cannot, in general, lock a single part of an object, because then you cannot have transactional semantics for the object as a whole.

Answer 2

Your design puts to onus on the user to ensure that the object is locked and unlocked at the correct times. And even though your locked object does error checking, it does not cover all bases (like forgetting to release the object when finished with it)

So lets say you have Unsafe object TData. You wrap this in Foo but instead of Foo returning a pointer to TData, reimplement all the public methods in TData in Foo but using locks and unlocks.

This is very similar to the pImpl pattern except your interface adds locks before calling the implementation. This way the user just knows the object is thread safe and does not need to worry about synchronization.

Answer 3

This is the core problem of multi-threading, you cannot demand that the client code uses your object in a thread-safe manner. Nor can you really do anything to help the client code fall into the pit of success, it has to take care of the locking by itself. Putting the onus of making your code work on the person that is least likely to get it right.

You can make it easier by returning a copy of the object from your accessor. That's thread-safe, there will only be one copy owned by one thread. You probably ought to make that copy's type immutable to re-inforce that modifying the object won't likely have the desired outcome. An unsolvable side-effect that might bite badly is that this copy is by definition stale. These are band-aids that are likely to do more harm than good.

Document the stuffing out of the method so that the client programmer knows what to do.

Answer 4

This isn't particularly safe. Nothing stops the user from getting things out of order:

locked_object<T> o(...);
T* t = o.get();
o.unlock();
t->foo(); // Bad!

Of course, it's easy to see why the above code is bad, but real code is much more complex, and there are many ways in which pointers could hang around after the lock is released that are much more difficult to pin down.