How can I return a scoped lock?

Question

Consider, say, a collection of account balances. And then you have a complex function that needs to check the balances of a few different accounts and then adjust the balances of a few different accounts. The operations need to be atomic with respect to other users of the collection. You have a collection class whose primary job is to provide this kind of atomicity. What's the 'right' way?

I have a class that has a boost::mutex member. The problem is that callers may need to perform a series of calls into the class while holding the mutex. But I don't want to give code outside the class free reign on the mutex.

What I'd like to do is something like this (pseudo-code):

class MyClass
{
 private:
  boost::mutex mLock;
 public:
  boost::scoped_lock& ScopedLock(return ScopedLock(mLock));
}

That way, callers can do this:

MyClass f;
if(foo)
{
 boost::scoped_lock lock(f->GetScopedLock());
 f->LockedFunc1();
 f->LockedFunc2();
}

The idea is that LockedFunc1 and LockedFunc2 would be called with the lock held. The destructor for lock would unlock f->mLock.

I have two basic questions:

1) How can I do this?

2) Is this sensible?

Note: This is completely different from this similarly named question: return a boost::scoped_lock.

Answer 1

I know its too late to respond to this question, however doing so, as it may add more context to someone who is facing a similar situation. How about approaching the solution like this:

class YourClass
{
    std::mutex m;
public:
    void YourAtomicFunction(std::function theUsersCallback)
    {
        std::unique_lock<std::mutex> l(m);
        theUsersCallback(any_data_which_needs_to_be_passed / this);
    }
};

Answer 2

This is how I presently plan to do it. I'll make a ScopedLock class that can be returned. To use it a class must have a boost::mutex and return ScopedLock constructed with that mutex. The caller uses the function to construct their own ScopedLock, and the caller's ScopedLock inherits the lock created by the class member function.

The pointer is safe because the ScopedLock cannot exceed the life of the class member whose member function you called to acquire it. And you are guaranteed (by the logic of the class) that there will only be one unlock.

The only real problem I see would be deliberate abuse. For example, if someone constructed a new ScopedLock off their ScopedLock, causing the other ScopedLock (possibly with a longer life) to inherit a lock it should not have. (It hurts when I do that. So don't do that.)

class ScopedLock {
private:
    boost::mutex *mMutex;   // parent object has greater scope, so guaranteed valid
    mutable bool mValid;

    ScopedLock();           // no implementation

public:
    ScopedLock(boost::mutex &mutex) : mMutex(&mutex), mValid(true) {
        mMutex->lock();
    }

    ~ScopedLock() {
        if(mValid) mMutex->unlock();
    }

    ScopedLock(const ScopedLock &sl) {
        mMutex=sl.mMutex;
        if(sl.mValid)
        {
                mValid=true;
                sl.mValid=false;
        }
        else mValid=false;
    }

    ScopedLock &operator=(const ScopedLock &sl)
    { // we inherit any lock the other class member had
        if(mValid) mMutex->unlock();
        mMutex=sl.mMutex;
        if(sl.mValid) {
            mValid=true;
            sl.mValid=false;
        }
    }
};

It still feels kind of wrong to me. I thought the whole point of Boost was to provide a clean interface for all the things you are most likely to need to do. This seems very routine to me. And the fact that there's no clean way to do it scares me.

Update: The "correct" Boost way to do this is to use a shared_ptr to a lock holder object. The object will go away when its last pointer is destroyed, releasing the lock.

Answer 3

How can I do this?

Alternative 1

One approach would be to create a type which has a boost::scoped_lock:

class t_scope_lock {
public:
  t_scope_lock(MyClass& myClass);
  ...
private:
  boost::scoped_lock d_lock;
};

and for MyClass to grant access to the mutex for this type. If this class is written specifically for MyClass, then I'd just add it as an inner class MyClass::t_scoped_lock.

Alternative 2

Another approach would be to create an intermediate type for use with the scope lock which could be convertible to a (custom) scope lock's constructor. Then the types could opt in as they see fit. A lot of people may not like the custom scope lock, but it would allow you to easily specify the access as you desire, and with a good degree of control.

Alternative 3

Sometimes it's better to add an abstraction layer for MyClass. If the class is complex, this is not likely a good solution because you will need to provide a lot of variants which look like:

{
 boost::scoped_lock lock(f->GetScopedLock());
 f->LockedFunc1();
 f->LockedFunc2();
}

Alternative 4

Sometimes you can use another lock (e.g. internal and external).

Alternative 5

Similar to #4, you can use a recursive or readwrite lock in some cases.

Alternative 6

You can use a locked wrapper type to selectively grant access to portions of the type's interface.

class MyClassLockedMutator : StackOnly {
public:
    MyClassLockedMutator(MyClass& myClass);
// ...
    void LockedFunc1() { this->myClass.LockedFunc1(); }
    void LockedFunc2() { this->myClass.LockedFunc2(); }
private:
    MyClass& myClass;
    boost::scoped_lock d_lock; // << locks myClass
};

MyClass f;
MyClassLockedMutator a(f);

a.LockedFunc1();
a.LockedFunc2();

Is this sensible?

Keep in mind that I have no idea what the exact constraints of your program are (hence, multiple alternatives).

Alternatives #1, #2, #3, and #6 have (virtually) no performance overhead, and have marginal additional complexity in many cases. They are, however, syntactically noisy for a client. IMO, forced correctness which the compiler can check (as needed) is more important than minimizing syntactical noise.

Alternatives #4 and #5 are good ways to introduce additional overhead/contention or locking/concurrent errors and bugs. In some cases, it is a simple substitution worth consideration.

When correctness, performance, and/or other restrictions are critical, I think it makes perfect sense to abstract or encapsulate those complexities, even if it costs some syntactical noise or an abstraction layer. I do this because it's too easy introduce breaking changes - even if I have written and maintained the entire program. To me, it's a more elaborate case of visibility, and perfectly sensible if used correctly.

Some Examples

Scroll down to main - this sample is rather disorganized because it demonstrates several approaches in one:

#include <iostream>
#include <boost/thread.hpp>

class MyClass;

class MyClassOperatorBase {
public:
    /* >> public interface */
    bool bazzie(bool foo);
protected:
    MyClassOperatorBase(MyClass& myClass) : d_myClass(myClass) {
    }

    virtual ~MyClassOperatorBase() {
    }

    operator boost::mutex & ();

    MyClass& getMyClass() {
        return this->d_myClass;
    }

    const MyClass& getMyClass() const {
        return this->d_myClass;
    }

protected:
    /* >> required overrides */
    virtual bool imp_bazzie(bool foo) = 0;
private:
    MyClass& d_myClass;
private:
    /* >> prohibited */
    MyClassOperatorBase(const MyClassOperatorBase&);
    MyClassOperatorBase& operator=(const MyClassOperatorBase&);
};

class MyClass {
public:
    MyClass() : mLock() {
    }

    virtual ~MyClass() {
    }

    void LockedFunc1() {
        std::cout << "hello ";
    }

    void LockedFunc2() {
        std::cout << "world\n";
    }

    bool bizzle(bool foo) {
        boost::mutex::scoped_lock lock(this->mLock);

        return this->imp_bizzle(foo);
    }

protected:
    virtual bool imp_bizzle(bool foo) {
        /* would be pure virtual if we did not need to create it for other tests. */
        return foo;
    }

private:
    class t_scope_lock {
    public:
        t_scope_lock(MyClass& myClass) : d_lock(myClass.mLock) {
        }

    private:
        boost::mutex::scoped_lock d_lock;
    };
protected:
    friend class MyClassOperatorBase;
private:
    boost::mutex mLock;
};

MyClassOperatorBase::operator boost::mutex & () {
    return this->getMyClass().mLock;
}

bool MyClassOperatorBase::bazzie(bool foo) {
    MyClass::t_scope_lock lock(this->getMyClass());

    return this->imp_bazzie(foo);
}

class TheirClassOperator : public MyClassOperatorBase {
public:
    TheirClassOperator(MyClass& myClass) : MyClassOperatorBase(myClass) {
    }

    virtual ~TheirClassOperator() {
    }

    bool baz(bool foo) {
        boost::mutex::scoped_lock lock(*this);

        return this->work(foo);
    }

    boost::mutex& evilClientMove() {
        return *this;
    }

protected:
    virtual bool imp_bazzie(bool foo) {
        return this->work(foo);
    }

private:
    bool work(bool foo) {
        MyClass& m(this->getMyClass());

        m.LockedFunc1();
        m.LockedFunc2();
        return foo;
    }
};

class TheirClass : public MyClass {
public:
    TheirClass() : MyClass() {
    }

    virtual ~TheirClass() {
    }

protected:
    virtual bool imp_bizzle(bool foo) {
        std::cout << "hallo, welt!\n";
        return foo;
    }
};

namespace {
/* attempt to restrict the lock's visibility to MyClassOperatorBase types. no virtual required: */
void ExampleA() {
    MyClass my;
    TheirClassOperator their(my);

    their.baz(true);

// boost::mutex::scoped_lock lock(my); << error inaccessible
// boost::mutex::scoped_lock lock(my.mLock); << error inaccessible
// boost::mutex::scoped_lock lock(their); << error inaccessible

    boost::mutex::scoped_lock lock(their.evilClientMove());
}

/* restrict the lock's visibility to MyClassOperatorBase and call through a virtual: */
void ExampleB() {
    MyClass my;
    TheirClassOperator their(my);

    their.bazzie(true);
}

/* if they derive from my class, then life is simple: */
void ExampleC() {
    TheirClass their;

    their.bizzle(true);
}
}

int main(int argc, const char* argv[]) {
    ExampleA();
    ExampleB();
    ExampleC();
    return 0;
}

Answer 4

The preferred solution would be an atomic function like this:

void MyClass::Func_1_2( void )
{
   boost::lock_guard<boost::mutex> lock( m_mutex );
   LockedFunc1();
   LockedFunc2();
}

You may have to provide several of these extra methods. The principle: is it's better to hide your locking policies from the user. If you find that it's not reasonable to create the special methods, you may want to rethink your design, abstract at a higher level.

If you have legitimate reasons to keep the interface the same, hide the locking details behind helper classes. Two examples.

Hide the lock behind a token class that is passed to methods that require a lock.

MyClass my_class;

{
   LockMyClass locked( my_class );
   myclass.Func1( locked ); // assert or throw if locked is not locking my_class
   myclass.Func2( locked );
}

Create a locked interface class that is a friend of MyClass:

MyClass my_class;

{
   LockedMyClass locked( my_class );
   locked.Func1(); 
   locked.Func2();
}

Is this sensible?

If you're careful it can be done but generally you don't want to expose your synchronization details outside of your class. There are just too many problems that can arise. Sun tried a similar idea with java.util.Vector, but has since moved onto better techniques.