Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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
share|improve this question
5  
<quote>I found that my solution was superior in several aspects</quote> Brave words. –  Loki Astari Sep 14 '12 at 23:14
    
@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... –  Gabriel Sep 15 '12 at 20:57
    
You could change T* into T but that brings another whole set of problems (just off the top of my head slicing). –  Loki Astari Sep 15 '12 at 21:07
    
Looks like it compiles now. I'll have a deep look tonight. –  Loki Astari Sep 17 '12 at 20:14
    
@LokiAstari Thanks for the compile errors! –  Gabriel Sep 19 '12 at 14:05
show 1 more comment

3 Answers

up vote 1 down vote accepted

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);
}
share|improve this answer
    
As suggested on SO, I removed the const from inline LockedProxy(const LockedProxy && other). Hopefully it works now. –  Gabriel Sep 17 '12 at 17:48
    
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". –  Gabriel Sep 24 '12 at 21:47
    
Ah, and the conversion to bool allows for testing after TryGetLockedProxy(), which may fail. –  Gabriel Sep 24 '12 at 21:54
add comment

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

share|improve this answer
    
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! –  Gabriel Sep 19 '12 at 14:26
1  
I like the bool. Throwing exceptions should be, by definition, reserved to exceptional cases. –  Gabriel Sep 19 '12 at 16:56
    
Here we go with the new version. –  Gabriel Sep 19 '12 at 17:56
add comment

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.

share|improve this answer
    
What is this a comment on something else? –  Loki Astari Sep 20 '12 at 17:26
1  
A possible improvement. I'm waiting for up/downvotes to consider implementing it or not. Is talking to oneself using the second person unusual ? (; –  Gabriel Sep 21 '12 at 21:38
    
Done. (10 more chars to go) –  Gabriel Feb 18 '13 at 12:48
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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