ScopedLock & CriticalSection question

I just noticed that I have some code similar to the following and was wondering why it worked:

class Class
{
    void foo()
    {
        ScopedLock lock(criticalSection);
        // ...
    }
    void bar()
    {
        ScopedLock lock(criticalSection);
        // ...
        foo();
        // ...
    }

    CriticalSection criticalSection;
};

In bar() a ScopedLock is used and later foo() is called. What I’d expect is that the lock in foo() can’t enter the criticalSection mutex as it is already used in bar() but it simply works. A further thought would be that the mutex is thread-dependent and the lock in foo() therefore doesn’t block. But in that case the mutex might be released twice (when exiting foo() and again when exiting bar() which also doesn’t sound very good.

I will rewrite the code so foo() can be called without creating a lock in case the mutex is already in use but I was wondering whether the above code is not as bad as it looks to me right now.

This is on Windows if this might make any difference.

It depends whether you call foo() and bar() from different threads. If the critical section isn’t locked and you call bar() then when the foo() is called in turn and the lock succeeds (as it is already locked by the that thread). On Mac, Linux and other POSIX the critical sections use PTHREAD_MUTEX_RECURSIVE so a lock count is maintained to allow this to work. (The Windows Enter/LeaveCriticalSection functions that JUCE uses use the same principle.)

If the critical section is already locked by another thread then calling either function on another thread will block.

1 Like

Thanks for the reply. So if I understand correctly, the above code as such is OK and can be expected to work on Windows as well as Unix/Linux.
Is this good style?

Is this good style?

Why not?

For the unknowing eye (e.g. me three hours ago :slight_smile: ) it might appear prone to break as one lock might block the other. The reason that it works doesn’t necessarily mean that it is the best implementation.

But it happens on the same thread, so (theoretically) there can’t be a race condition, because you now what you call inside the scoped-lock.
Other kind of locks may behave differently, for example Juce’s SpinLock does not allow re-entrance.

(is it just me or “chkn” and “ckhf” are way too similar as usernames? without a telling profile picture, I was surprised to see the expert “chkn” asking such a basic question about locks, only to realize when he actually chimed in, that the OP was another user instead :joy:)

So the short answer is: No. This is not good design.

The longer answer depends on the implementation of the locking mechanism itself and (as it has been pointed out) whether or not the lock is re-entrant.

A better design is as follows: use a public interface that acquires the lock(s) and private methods that perform the actions. The private implementations never call the public methods.

[code]
class CriticalResource
{
public:

// Public Interface
void foo(){
    ScopedLock lock(criticalSection);
    _foo();
}

void bar(){ 

    ScopedLock lock(criticalSection);
    _bar();
}

private:
CriticalSection criticalSection;

private: // Internal Implementation
void _foo() { _bar(); }
void _bar() { … }
}[/code]

This also makes it easier if you need to solve lock-order related race conditions (Deadlock). In those situations, you create a well defined ‘order-of-the-locks’ that you always acquire in the same order. Even if you aren’t acquiring all of the locks.

[code]
// Well defined locking order: ALWAYS acquire locks in this order!
// FOO_LOCK
// BAR_LOCK
class CriticalResource
{
public:

// Public Interface
void foo(){
    ScopedLock foo_lock(fooSection);
    ScopedLock bar_lock(barSection); // _foo() uses bar resource
    _foo();
}
void bar(){ 
    ScopedLock bar_lock(barSection);
    _bar();
}

private:
CriticalSection fooSection;
CriticalSection barSection;

private: // Internal Implementation
void _foo() { /Foo Resource/ … ; _bar(); }
void _bar() { /Bar Resource/ … }
}[/code]

When a lock fails to acquire, it goes to sleep and waits for a signal. As long as the locks are always acquired in the same order (regardless of which locks are and are not used), this will prevent a deadlock situation.