How can I return a scoped lock?
Asked Answered
H

4

14

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.

Huckaback answered 11/11, 2011 at 4:31 Comment(3)
I stumbled upon this looking for a different issue. But, what strikes me is that passing the lock out of MyClass is really the opposite of what you want to do. Instead, what you really want is a method on MyClass called "ExecuteAtomically" or "ExecuteLocked" to which you can pass some sort of lambda expression that contains whatever chunk of code you want evaluated in the context of having the lock held. I'm trying to figure out the best way to do that with Boost/C++, but maybe somebody smarter than me could chime in.Seamark
@ChrisCleeland: That is a very interesting way to think about the problem.Huckaback
Thought about it a little more, and I think it could be done. The "Execute" method could have a signature something like Execute(boost::function<void (MyClass&)>) so that you could define a functor and bind "this" (or *this) for the 1st argument. Presuming that works properly (which I think it will), it shouldn't be too much more work to employ boost::lambda (or the new language feature) to move the body of the functor so that it's in-place with the call to Execute(). I had hoped I could figure out a place in my current task list to try this out, but I haven't. May try later.Seamark
X
11

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;
}
Xiaoximena answered 11/11, 2011 at 6:1 Comment(4)
I'll have to see if I can make that work. I'm not sure how the t_scope_lock constructor would get access to the lock. Would the classes all have to descend from some kind of 't_scope_lockable' class so the t_scope_lock constructor can find their lock? (I can live with some ugliness in the classes if the callers get the clean syntax in the question.)Huckaback
Also, can you answer question 2 if possible? Why doesn't everyone do this? It seems a natural way to operate on a class that has to be stable over multiple operations. (Get accessor object. Check information X and Y on it, make a decision, modify the object through the accessor. Done, release lock.) Am I thinking about my (very typical, I think) problem wrong? Or does everyone else do this some awful way?Huckaback
@David re Comment #1: I've expanded the response (added alt.4-6), but I'll produce a sample. re Comment #2: Sorry, forgot that bit by the time I'd answered the first - response updated. I will go to even greater lengths to ensure correctness and speed, allowing the compiler to make some of the checks (programs are too large and update too quickly to trust myself). I'm not sure why I haven't seen this often - perhaps because it takes time to learn to implement, and for clients to use. To me, it requires less time (overall) to abstract the more complex cases.Xiaoximena
@David i have added three examples.Xiaoximena
M
0

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.

Methadone answered 11/11, 2011 at 17:52 Comment(3)
What's the better solution when you have a class that represents a collection and all the clients of that class need to perform very complex operations on members of that collection (Find X, Find Y, make decision, modify X, delete Y, ...) and you don't need much concurrency, so a single lock on the collection is adequate?Huckaback
What you describe sounds like the lock should exist outside of the container. So yes a single lock sounds correct. Or you may be able to add a method to the container MyClass::make_decision( x, y ) which would handle the whole process.Methadone
Okay, the lock exists outside the container. Now, I have the same problem, just "the container" is now "wherever the lock exists". The class can't make the decision because it requires frequent access to information way outside the scope of the class. (Consider a low-level 'object state' class and 'collection of object states' class being operated on by numerous sophisticated calling classes that operate on related states of multiple objects. The purpose of the container is specifically to implement a consistent state across operations until the caller is finished.)Huckaback
H
0

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.

Huckaback answered 12/11, 2011 at 0:36 Comment(1)
Could you update your example to reflect the correct boost way of doing this? I'm not completely clear on it.Stuffy
D
0

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);
    }
};
Donata answered 1/7, 2024 at 11:15 Comment(2)
This only works for member functions. From the problem description: "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."Huckaback
"The problem is that callers may need to perform a series of calls into the class while holding the mutex." caller's pseudo code: ' void CallsWhichNeedToPerformedWhileHoldingTheMutex() { Call1(); Call2(); Calln(); } void main() { YourClass * p = new YourClass(); p->YourAtomicFunction( CallsWhichNeedToPerformedWhileHoldingTheMutex ); } 'Donata

© 2022 - 2025 — McMap. All rights reserved.