HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Force locking for thread-safety

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
forcethreadforsafetylocking

Problem

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
#include

template
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 lock;
T * t;

Solution

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 GetLockedProxy() {
    return LockedProxy(mutex, &lockableObject);
}

Code Snippets

inline LockedProxy(boost::mutex & m, T * obj)
    :lock(m),
    t((lock)?obj:nullptr)
{
}
inline LockedProxy(LockedProxy && other)
inline operator bool() const
{
    return lock;
}
LockedProxy&   x  = /* Get a lock proxy */;

int y = 10 + x;  // Will now compile fine (you probably want a compile error here).
inline Lockable(const T & t)
    :lockableObject(t)
{}

Context

StackExchange Code Review Q#15632, answer score: 8

Revisions (0)

No revisions yet.