15
\$\begingroup\$

After reading Herb Sutter's Associate Mutexes with Data to Prevent Races, I found that my solution was superior in several aspects, least important first:

  • The code is cleaner, without macros
  • No code added to every struct/class
  • Does not prevent the use of non-protected versions of the classes/structs
  • Does not rely on code coverage, which is subject to omission (relies on the compiler instead)

The idea is to never give access to an object which associated mutex has not been locked. The locking is still explicit, but without it, one simply can't do anything with the objects. One passes around Lockable. Lockable has a mutex and a T. To be able to access T, one has to call Lockable::GetLockedProxy() that returns a LockedProxy object. A LockedProxy has a scoped_lock and a T*, and allows operations on the T.

To access the T, one uses LockedProxy::operator->() or operator*().

While a LockedProxy is alive, that is, while one has access to the T, the associated scoped_lock is alive, meaning that the mutex is locked, and subsequent calls to GetLockedProxy, which will try to create a scoped_lock, will hang.

The drawback is, one still can be nasty and Lock the object and get a pointer to it (via LockedProxy's operator*) and store it, then release the lock and use the pointer. That's the abstraction's leak. But that's pointing the gun to one's foot and pulling the trigger.

The classes: Lockable.h

#include <boost/thread/mutex.hpp>
#include <boost/interprocess/sync/scoped_lock.hpp>

template<typename T>
class LockedProxy : boost::noncopyable
{
public:
    inline LockedProxy(boost::mutex & m, T * obj)
        :lock(m),
        t(obj)
    {}
    inline LockedProxy(LockedProxy && other)
        :lock(std::move(other.lock)),
        t(std::move(other.t))
    {}

    inline       T * operator->()       { return t; }
    inline const T * operator->() const { return t; }

    inline const T & operator*() const { return *t; }
    inline       T & operator*()       { return *t; }

private:
    boost::interprocess::scoped_lock<boost::mutex> lock;
    T * t;
};


template<typename T>
class Lockable
{
public:

    // Convenience typefed for subclasses to use
    typedef T LockableObjectType;

    inline Lockable(const T & t)
        :lockableObject(t)
    {}

    inline LockedProxy<LockableObjectType> GetLockedProxy() {
        return LockedProxy<LockableObjectType>(mutex, &lockableObject);
    }

protected:
    LockableObjectType lockableObject;
    boost::mutex mutex;
};

How to use them:

#include <iostream>
#include <string.h>
#include "Lockable.h"

void f(Lockable<std::string> & str)
{
    auto proxy = str.GetLockedProxy();

    *proxy = "aa";
    proxy->append("bb");

    std::cout << "str = " << *proxy << std::endl;
}

void g(Lockable<int> & i)
{
    { // reduce lock's lifespan
        auto proxy = i.GetLockedProxy();
        *proxy = 321;
    }

    // relock, lock lives for the statement
    std::cout << "i = " << *i.GetLockedProxy() << std::endl;
}

int main()
{
    Lockable<std::string> str("abc");
    //Can't use str here, it is not locked
    f(str);

    Lockable<int> i(123);
    g(i);

    return 0;
}

The usual question goes here: What do you think ?

Are there drawbacks or pitfalls I did not see ?

Do other libs like boost have something similar that I should use instead ?


EDIT:

The following code implements @useless', @Loki Astari's and my own suggestions and should compile with both VS and g++.

I renamed Lockable and LockedProxy into Synchronized and SynchronizedProxy, for I found the former could lead to misunderstanding.

The classes have gronw some hair, but their use have not, which is what matters. Using proxy after TryGetSynchronizedProxy failed is equivalent to using a nullptr which throws, which is good.

Getting rid of the dependency on boost was a bit tricky. TryLock has some caveats in C++ 11 as described here, which motivates the use of recursive_mutex as default.

boost' safe bool is no longer required thanks to this, but it does not compile with VS2012, even with the november 2012 compiler update.

Synchronized.hpp:

#include <mutex>

template<typename T, class Mutex>
class SynchronizedProxy
{
    template<typename X, class Y>
    friend class Synchronized;

private:
    SynchronizedProxy();
    SynchronizedProxy(const SynchronizedProxy &);
    SynchronizedProxy & operator=(const SynchronizedProxy &);
    SynchronizedProxy & operator=(SynchronizedProxy &&);

    SynchronizedProxy(Mutex & m, T & obj)
        :lock(m)
        ,t(&obj)
    {}
    SynchronizedProxy(Mutex & m, T & obj, int)
        :lock(m, std::try_to_lock)
        ,t((lock)?&obj:nullptr)
    {}

public:
    SynchronizedProxy(SynchronizedProxy && other)
        :lock(*other.lock.mutex(), std::adopt_lock)
        ,t(std::move(other.t))
    {
        other.t = nullptr;
    }

    explicit operator bool() const { return (bool)lock; }

    const T * operator->() const { return t; }
          T * operator->()       { return t; }

    const T & operator*() const { return *t; }
          T & operator*()       { return *t; }

private:
    std::unique_lock<Mutex> lock;
    T * t;
};


template<typename T, class Mutex=std::recursive_mutex>
class Synchronized
{
public:
    // Convenience typefed for subclasses to use
    typedef T SynchronizedObject;

    Synchronized(const T & t)
        :t(t)
    {}

    SynchronizedProxy<T,Mutex> GetSynchronizedProxy() {
        return SynchronizedProxy<T,Mutex>(mutex, t);
    }
    SynchronizedProxy<T,Mutex> TryGetSynchronizedProxy() {
        return SynchronizedProxy<T,Mutex>(mutex, t, 0);
    }

protected:
    T t;
    Mutex mutex;
};

main.cpp:

#include <iostream>
#include <thread>
#include <string>
#include "Synchronized.hpp"

void f(Synchronized<std::string> & str)
{
    auto proxy = str.GetSynchronizedProxy();

    *proxy = "aa";
    proxy->append("bb");

    std::cout << "str = " << *proxy << std::endl;
}

void g(Synchronized<int> & i)
{
    { // reduce lock's lifespan
        auto proxy = i.GetSynchronizedProxy();
        *proxy = 321;
    }

    // relock, lock lives for the statement
    std::cout << "i = " << *i.GetSynchronizedProxy() << std::endl;
}

int main()
{
    Synchronized<std::string> str("abc");
    //Can't use str here, it is not locked
    f(str);

    Synchronized<int> i(123);
    g(i);

    {
        auto proxy = i.TryGetSynchronizedProxy();
        if (proxy)
        {
            *proxy = 222;
        }
    }

    {
        auto p = i.GetSynchronizedProxy();

        std::thread([&i]
            {
                auto proxy = i.TryGetSynchronizedProxy();
                if (proxy)
                {
                    *proxy = 333;
                }
            }
        ).join();
    }

    std::cout << "i = " << *i.GetSynchronizedProxy() << std::endl;

    return 0;
}

Output:

str = aabb
i = 321
i = 222
\$\endgroup\$
4
  • 7
    \$\begingroup\$ <quote>I found that my solution was superior in several aspects</quote> Brave words. \$\endgroup\$ Commented Sep 14, 2012 at 23:14
  • \$\begingroup\$ @LokiAstari One could use a T instead of a T* inside LockedProxy. When locking, move the T from Lockable to LockedProxy, and move it back when unlocking. I think that would not be very efficient, though... \$\endgroup\$
    – Gabriel
    Commented Sep 15, 2012 at 20:57
  • \$\begingroup\$ You could change T* into T but that brings another whole set of problems (just off the top of my head slicing). \$\endgroup\$ Commented Sep 15, 2012 at 21:07
  • \$\begingroup\$ Turns out that Mr. Edd had a similar solution a year earlier (this was posted in 2012 but developped in 2011), as he posted in 2010 in Sutter's Mill's comments: herbsutter.com/2010/05/24/… , and on his own site: mr-edd.co.uk/blog/associate_mutexes_to_prevent_races \$\endgroup\$
    – Gabriel
    Commented Sep 27, 2012 at 13:44

4 Answers 4

8
\$\begingroup\$

Since the only place you should be creating a LockedProxy from is within the call Lockable::GetLockedProxy() you should therefore make the constructor private and friend this function. You don't actually want to give people the ability to make objects of this type themselves.

inline LockedProxy(boost::mutex & m, T * obj)
    :lock(m),
    t((lock)?obj:nullptr)
{
}

Also since the access to this constructor is so controlled you never really want to pass a NULL object so you should pass obj by reference. If you must store it internally as a pointer fine take the address of the reference parameter for local storage but personally I would maintain everything as references.

You should not be using the keyword inline unless you are required to do so. Declaring and defining it inside the class makes it automatically tagged inline. And apart from linking it has no affect on the compiler. So avoid this keyword unless you actually need in and you don't (it just clutters the code and makes people think you are trying to inline code (which only happens if the compiler thinks it is required)).

Does it make sense to be able to move the Locked Proxy?

inline LockedProxy(LockedProxy && other)

Not sure. I don't understand the use case where you would want or need to do that.

Auto conversion to bool?

inline operator bool() const
{
    return lock;
}

Why? Again I don't see the use case. Also you should probably look up the safe bool idiom. The problem with bool is that is auto converted to integer so the compiler will now be able to compile your code what it see this:

LockedProxy&   x  = /* Get a lock proxy */;

int y = 10 + x;  // Will now compile fine (you probably want a compile error here).

Rather than pass the object in. You should look up variadic templates.
This will allow your constructor here to take the same arguments as the T and forward them directly to the T object being constructed. So now the only T is the one inside the object.

inline Lockable(const T & t)
    :lockableObject(t)
{}

Same comments about inline as above.

inline LockedProxy<LockableObject,Mutex> GetLockedProxy() {
    return LockedProxy<LockableObject,Mutex>(mutex, &lockableObject);
}
\$\endgroup\$
3
  • \$\begingroup\$ As suggested on SO, I removed the const from inline LockedProxy(const LockedProxy && other). Hopefully it works now. \$\endgroup\$
    – Gabriel
    Commented Sep 17, 2012 at 17:48
  • \$\begingroup\$ Private + friend: OK. Pass-by-reference: OK. inline: OK. The move constructor in LockedProxy enables returning in GetLockedProxy(). Safe bool idiom: OK (nice one, i'll sleep less dumb tonight). I'll have to pass on the variadic templates, VS won't compile them. Many thanks, "that was very educational". \$\endgroup\$
    – Gabriel
    Commented Sep 24, 2012 at 21:47
  • \$\begingroup\$ Ah, and the conversion to bool allows for testing after TryGetLockedProxy(), which may fail. \$\endgroup\$
    – Gabriel
    Commented Sep 24, 2012 at 21:54
5
\$\begingroup\$

Herb suggested another way for associating mutexes with data in c++11 more recently than the linked article in this video. The whole video is worth watching in my opinion, but he explains a wrapper pattern around minute 38 and shows how it applies to associating a mutex in minute 40. His implementation looks like this:

template< class T > class monitor {
private:
    mutable T t;
    mutable std::mutex m;

public:
    monitor( T t_ = T{} ) : t(t_) {}

    template< typename F >
    auto operator()( F f ) const -> decltype(f(t))
        { std::lock_guard<std::mutex> _{m}; return f(t); }
};

You then use the wrapped object it by passing a callable object that takes by reference the object being synchronized. Here is a brief example:

monitor<string> s;
s([]( string &s ) {
    s.append( "syncrhonized append" );
});

I think it is a pretty slick way of forcing synchronization which makes the synchronization a little more visually explicit than returning a proxy. He gave a minimal implementation for his slide. My take on it, hopefully, made it a bit more useful. It...

  1. Abstracts the mutex type to anything that models the BasicLockable concept.
  2. Uses empty base class optimization to combine the BasicLockableT with the protected value. This preserves space in the case that you have an empty BasicLockableT.
  3. Forwards all constructor arguments to the protected value. This allows you to construct T with any of T's constructors.
  4. Overloads the sync function to preserve constness of the protected value.

Here it is:

template< typename T, typename BasicLockableT = std::mutex >
class Synchronized
{
    struct ProtectedValue : BasicLockableT {
        template< typename ...Args >
        ProtectedValue ( Args&&... params )
            : value( std::forward<Args>(params)... )
        {}

        BasicLockableT& lockable()
        { return *this; }

        T value;
    };
    mutable ProtectedValue m_protected_value;

public:
    template< typename ...Args >
    Synchronized( Args&&... params )
        : m_protected_value( std::forward<Args>(params)... )
    {}

    template< typename F >
    auto sync( F f ) -> decltype( f( m_protected_value.value ) )
    {
        std::lock_guard< BasicLockableT > lock( m_protected_value.lockable() );
        return f( m_protected_value.value );
    }

    template< typename F >
    auto sync( F f ) const
        -> decltype( f( const_cast<T const &>(m_protected_value.value) ) )
    {
        std::lock_guard< BasicLockableT > lock( m_protected_value.lockable() );
        return f( const_cast<T const &>(m_protected_value.value) );
    }
};

Anyway I just wanted to throw a c++11 way of doing this out there.

\$\endgroup\$
0
4
\$\begingroup\$

The only weaknesses I can see immediately are that:

  1. you're limited to the mutex type you've hard-coded, although I'm sure you could template it out (read-write locks would probably require another, read-only, proxy class to be useful)

  2. no support for try-lock operations: again this probably requires another proxy type

    • I was thinking it could simply call try_lock and record the boolean success/failure status,
      • expose the success or failure by conversion to bool
      • and be convertible to a regular locked proxy (only in case if success) using the adopt tag

    eg.

        auto maybe = lockable.trylock();
        if (maybe) { auto proxy = maybe.proxy(); // normal locked proxy use ...
        } else ; // failed to lock ...
    
    • alternatively of course, it could just throw if it fails
  3. your LockedProxy move constructor should probably reset other.t = NULL explicitly

\$\endgroup\$
2
  • \$\begingroup\$ 1. Indeed that should be easy. Maybe with nice default values? 2. Interesting! What would GetTryLockProxy do on failure? Return a TryLockProxy that compares true with nullptr? 3. True! \$\endgroup\$
    – Gabriel
    Commented Sep 19, 2012 at 14:26
  • 1
    \$\begingroup\$ I like the bool. Throwing exceptions should be, by definition, reserved to exceptional cases. \$\endgroup\$
    – Gabriel
    Commented Sep 19, 2012 at 16:56
1
\$\begingroup\$

Remove the dependency on boost. Not everybody uses it. Although the way you are using the mutex, its subtype scoped_try_lock and scoped_lock are very boost-like, any kind of mutex and lock should be allowed, as not everybody can or want to use boost.

\$\endgroup\$
2
  • \$\begingroup\$ What is this a comment on something else? \$\endgroup\$ Commented Sep 20, 2012 at 17:26
  • 1
    \$\begingroup\$ A possible improvement. I'm waiting for up/downvotes to consider implementing it or not. Is talking to oneself using the second person unusual ? (; \$\endgroup\$
    – Gabriel
    Commented Sep 21, 2012 at 21:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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