A scoped lock/unlock implementation in C++.
JC
got me thinking about an implementation that could get around some of
problems discussed in that thread.
Define a "scoped lock" as an object that acquires a lock on a mutex at
initialization and releases the lock when it goes out of scope, and a
"scoped unlock" as an object that releases a lock on a mutex at
initialization and acquires the lock when it goes out of scope. Note
that these definitions do not require the lock to be held the entire
time a "scoped lock" is in scope, nor do they require that no lock is
held the entire time an "unscoped lock" is in scope (this was a point
of debate in the aforementioned thread).
The specific problem this addresses is nested scoped locks and
unlocks. For example, in the following situation:
{
scoped_lock locker(g_mutex);
{
scoped_lock locker(g_mutex);
{
scoped_unlock unlocker(g_mutex);
// <-- if g_mutex supports recursive locks, then
// it actually remains locked here.
}
}
}
The implementation below guarantees that "unlocker" will release the
lock on the mutex.
The implementation also provides debugging asserts to flag various
mistakes. Comments are in the code below.
One issue that was brought up is the locking/unlocking of the mutex
during stack unwinding after an exception is thrown. This
implementation does not "solve" that problem. That problem is, in
fact, unavoidable, and present in any scope-based locking scheme. The
reason it is unavoidable is because inner scopes do not have knowledge
of how outer scopes will handle exceptions, and outer scopes must be
able to make correct assumptions about the state of the mutex when
handling exceptions. Therefore, "optimizations" such as skipping a
transient lock during stack unwinding are simply not possible without
breaking assumptions about the mutex's state along the way.
Here is the implementation I came up with. It defines the concept of
locked and unlocked "regions". It uses an std::vector<int> to track
the nesting level of regions. It also provides "unscoped" locking and
unlocking for completeness. In the code below, a simple benchmark of
these two loops:
static const int ITERS = 100000;
mutex m;
mutex_locker ml(m);
int n;
for (n = 0; n < ITERS; ++ n) {
m.lock();
m.unlock();
}
for (n = 0; n < ITERS; ++ n) {
ml.enter_locked();
ml.leave_locked();
}
Yielded the following times when compiled with MSVC 2008 and run on a
2.16GHz Intel T2600 Core Duo running Windows XP SP3:
Release Mode:
mutex: 3.7393 ms / 100000 iters.
mutex_locker: 5.6954 ms / 100000 iters.
Debug Mode:
mutex: 15.7196 ms / 100000 iters.
mutex_locker: 1000.9 ms / 100000 iters.
The performance of the mutex_locker in release mode is worse than the
mutex, but may be acceptable for many applications. The performance of
the mutex_locker in debug mode is significantly worse, due mainly to
debug assertions in the code. Improvements to release mode performance
are welcome. Note that the use of an std::list rather than an
std::vector increased the release mode mutex_locker time to approx.
100 ms per 100000 iterations.
Here is the code. However, as I suspect Google Groups will break the
lines in awkward places, I have also placed the code here:
6 classes are provided:
mutex - A basic mutex.
mutex_locker - Provides lock/unlock "region" functionality.
scoped_lock, scoped_unlock - Scope-based region management.
unscoped_lock, unscoped_unlock - Provided for completeness.
Details are in source comments. Suggestions/criticism welcome. This
was mostly done as an exercise. Note that while I put pthreads support
in there, I did not actually try to compile it on a POSIX system --
apologies for any mistakes.
Jason
#define USE_WIN_ASSERT 1
#define USE_STD_ASSERT 2
#define USE_WIN_THREADS 1
#define USE_POSIX_THREADS 2
#if defined(_WIN32) && !defined(__CYGWIN__)
# define WIN32_LEAN_AND_MEAN
# include <windows.h>
# define USE_ASSERT USE_WIN_ASSERT
# define USE_THREADS USE_WIN_THREADS
#else
// Assume pthreads is present if we're not on Windows. Of course, this
will not
// always be correct. Adjust as needed.
# define USE_ASSERT USE_STD_ASSERT
# define USE_THREADS USE_POSIX_THREADS
#endif
#if (USE_ASSERT == USE_WIN_ASSERT)
# define RLOCK_ASSERT _ASSERTE
#elif (USE_ASSERT == USE_STD_ASSERT)
# include <cassert>
# define RLOCK_ASSERT assert
#endif
#include <iostream>
#include <stdexcept>
#include <vector>
using std::cout;
using std::endl;
#define ALLOW_TRACE 1
#if _DEBUG && ALLOW_TRACE
# include <algorithm>
std::ostream & operator << (std::ostream &s, const std::vector<int>
&v) {
std::copy(v.begin(), v.end(), std::ostream_iterator<int>(s, " "));
return s;
}
# define TRACE(out) cout << out << endl
#else
# define TRACE(out) do {} while (0)
#endif
//-----------------------------------------------------------------------------
// A basic mutex. Support for recursion is not required. Interprocess
// support is not provided (but could be if you wanted).
class mutex {
#if (USE_THREADS == USE_WIN_THREADS)
public:
mutex () throw () { InitializeCriticalSection(&mtx_); }
~mutex () throw () { DeleteCriticalSection(&mtx_); }
void lock () throw () { EnterCriticalSection(&mtx_); TRACE("lock"); }
void unlock () throw () { LeaveCriticalSection(&mtx_); TRACE
("unlock"); }
private:
CRITICAL_SECTION mtx_;
#elif (USE_THREADS == USE_POSIX_THREADS)
public:
mutex () throw () { pthread_mutex_init(&mtx_, NULL); }
~mutex () throw () { pthread_mutex_destroy(&mtx_); }
void lock () throw () { pthread_mutex_lock(&mtx_); TRACE("lock"); }
void unlock () throw () { pthread_mutex_unlock(&mtx_); TRACE
("unlock"); }
private:
pthread_mutex_t mtx_;
#endif
};
// Wraps a mutex and provides a concept of locked and unlocked regions
of
// code. A locked region is an area where the mutex is normally
locked, an
// unlocked region is an area where the mutex is normally unlocked.
This
// provides full nesting and recursion support.
//
// - All enter_locked must be matched by leave_locked.
// - All enter_unlocked must be matched by leave_unlocked.
// - Regions may be arbitrarily nested but may not overlap.
// - An unlocked region may not be the outermost region.
// - Entering an unlocked region will always unlock the underlying
// mutex, regardless of the nesting depth or types of regions it
// is nested in.
// - Entering a locked region will always lock the underlying mutex,
// regardless of the nesting depth or types of regions it is
// nested in.
//
// Debug assertions are raised if:
//
// - A region is left but never entered (leave_xxx called without
// corresponding enter_xxx).
// - An unlocked region is entered at the topmost level.
// - Regions overlap (e.g. enter_locked followed by leave_unlocked).
// - The mutex_locker is destroyed while still in a region.
//
// This class is intended for use only by a single thread, and as such
// is not thread-safe (although the underlying mutex is).
//
// Important note: Mutex locks may be acquired/released by the methods
of
// the classes "mutex" or "mutex_locker". Within a single thread, for
a
// given mutex, a mutex_locker must not exist between calls to
mutex::lock()
// and mutex::unlock(). Likewise, the members of mutex must not be
called
// directly if a mutex_locker exists. In other words, use of "mutex"
and
// "mutex_locker" members must not overlap. Otherwise, the results are
// undefined.
//
// Implementation notes:
//
// This class keeps track of region nesting levels and provides
recursive
// locking support for the mutex. The underlying mutex need not
support
// recursive locking. This class ensures that the mutex is only locked
or
// unlocked during transitions between different region types:
//
// - mutex::lock() is called when...
// - ...initially entering a locked region.
// - ...entering a locked region from an unlocked region.
// - ...leaving an unlocked region into a locked region.
//
// - mutex::unlock() is called when...
// - ...leaving the outermost locked region.
// - ...entering an unlocked region from a locked region.
// - ...leaving a locked region into an unlocked region.
//
// In all other cases, the nesting depth is simply tallied.
class mutex_locker {
public:
explicit mutex_locker (mutex &m) throw () : m_(m) { }
~mutex_locker () throw () { RLOCK_ASSERT(track_.empty()); };
private:
// Note: Use scoped_lock, scoped_unlock, unscoped_lock, and
// unscoped_unlock instead. Using these methods will not break any
// code, however using the other classes clearly states your
// intentions.
void enter_locked () throw ();
void leave_locked () throw ();
void enter_unlocked () throw ();
void leave_unlocked () throw ();
// Clients use these instead:
friend class scoped_lock;
friend class scoped_unlock;
friend class unscoped_lock;
friend class unscoped_unlock;
private:
mutex &m_;
std::vector<int> track_;
// The track_ vector tracks region recursion and nesting. The back
// of the vector represents the current region. Positive numbers
// indicate locked regions, negative numbers indicate unlocked
// regions. The absolute value of the number is the nesting depth
// of that region type. The following regions:
//
// enter_locked
// enter_locked
// enter_unlocked
// enter_locked
// enter_unlocked
// enter_unlocked
// enter_unlocked
// [here]
// leave_unlocked
// leave_unlocked
// leave_unlocked
// leave_locked
// leave_unlocked
// leave_locked
// leave_locked
//
// Produce the following vector at the point labeled [here]:
//
// 2 -1 1 -3
//
// An empty vector indicates that no region is active.
//
// In debug mode, it is conceivable to add more information to this
vector
// to track things such as the source file and line number where the
// current region was entered. This implementation does not do that.
};
// A scoped_lock provides automatic entering and leaving of a locked
// region corresponding to the scope it resides in. Note that all
// locked region rules of a mutex_locker apply. Also note that the
// locking region rules still apply when a region is left during
// stack unwinding as a result of an exception.
class scoped_lock {
public:
explicit scoped_lock (mutex_locker &ml) throw () : ml_(ml)
{ ml_.enter_locked(); }
~scoped_lock () throw () { ml_.leave_locked(); }
private:
mutex_locker &ml_;
};
// A scoped_unlock provides automatic entering and leaving of an
// unlocked region corresponding to the scope it resides in. Note
// that all unlocked region rules of a mutex_locker apply. Also
// note that the unlocking region rules still apply when a region
// is left during stack unwinding as a result of an exception.
class scoped_unlock {
public:
explicit scoped_unlock (mutex_locker &ml) throw () : ml_(ml)
{ ml_.enter_unlocked(); }
~scoped_unlock () throw () { ml_.leave_unlocked(); }
private:
mutex_locker &ml_;
};
// An unscoped_lock is provided for completeness. Entering and
// leaving of a locked region is not automatic. All locked region
// rules of a mutex_locker apply.
class unscoped_lock {
public:
explicit unscoped_lock (mutex_locker &ml) throw () : ml_(ml) { }
void enter_locked () throw () { ml_.enter_locked(); }
void leave_locked () throw () { ml_.leave_locked(); }
private:
mutex_locker &ml_;
};
// An unscoped_unlock is also provided for completeness. Entering
// and leaving of an unlocked region is not automatic. All unlocked
// region rules of a mutex_locker apply.
class unscoped_unlock {
public:
explicit unscoped_unlock (mutex_locker &ml) throw () : ml_(ml) { }
void enter_unlocked () throw () { ml_.enter_unlocked(); }
void leave_unlocked () throw () { ml_.leave_unlocked(); }
private:
mutex_locker &ml_;
};
//-----------------------------------------------------------------------------
// Enter a locked region. See mutex_locker for rules.
void mutex_locker::enter_locked () throw () {
if (track_.empty() || track_.back() < 0) {
// Entering topmost locked region, or entering from an unlocked
region.
track_.push_back(1);
m_.lock();
} else {
// Already in a locked region.
track_.back() ++;
}
TRACE("enter_locked: " << track_);
}
// Leave a locked region. See mutex_locker for rules.
void mutex_locker::leave_locked () throw () {
RLOCK_ASSERT(!track_.empty()); // in a locked or unlocked region.
RLOCK_ASSERT(track_.back() > 0); // in a locked region.
if ((-- track_.back()) == 0) {
// Leaving a locked region into an unlocked region.
m_.unlock();
track_.pop_back();
}
TRACE("leave_locked: " << track_);
}
// Enter an unlocked region. See mutex_locker for rules.
void mutex_locker::enter_unlocked () throw () {
RLOCK_ASSERT(!track_.empty()); // in a locked or unlocked region.
if (track_.back() > 0) {
// Entering from a locked region.
m_.unlock();
track_.push_back(-1);
} else {
// Already in an unlocked region.
track_.back() --;
}
TRACE("enter_unlocked: " << track_);
}
// Leave an unlocked region. See mutex_locker for rules.
void mutex_locker::leave_unlocked () throw () {
RLOCK_ASSERT(!track_.empty()); // in a locked or unlocked region.
RLOCK_ASSERT(track_.back() < 0); // in an unlocked region.
if ((++ track_.back()) == 0) {
// Leaving an unlocked region into a locked region.
track_.pop_back();
m_.lock();
}
TRACE("leave_unlocked: " << track_);
}
//-----------------------------------------------------------------------------
void test (mutex &m) {
mutex_locker locker(m);
// Comment out various lines below for assertion failures.
unscoped_lock ul(locker);
ul.enter_locked();
try {
{
scoped_unlock su1(locker);
scoped_unlock su2(locker);
{
scoped_lock sl1(locker);
scoped_lock sl2(locker);
scoped_lock sl3(locker);
unscoped_unlock uu(locker);
uu.enter_unlocked();
try {
{
scoped_unlock su3(locker);
{
scoped_lock sl4(locker);
cout << "Made it!" << endl;
// This exception can be placed anywhere in this
// function unless otherwise noted.
throw std::runtime_error("OOPS!");
}
}
} catch (...) {
// <--- Do not throw here.
uu.leave_unlocked();
throw;
}
uu.leave_unlocked();
}
}
} catch (...) {
// <--- Do not throw here.
ul.leave_locked();
throw;
}
ul.leave_locked();
}
int main () {
mutex m;
try {
test(m);
} catch (std::exception &x) {
cout << "caught: " << x.what() << endl;
}
}
JC
> #define ALLOW_TRACE 1
Note: Set this to 0 to disable obnoxious trace messages in debug
builds.
Jason
Marcel Müller
> The specific problem this addresses is nested scoped locks and
> unlocks. For example, in the following situation:
>
> {
> scoped_lock locker(g_mutex);
> {
> scoped_lock locker(g_mutex);
> {
> scoped_unlock unlocker(g_mutex);
> // <-- if g_mutex supports recursive locks, then
> // it actually remains locked here.
> }
> }
> }
>
> The implementation below guarantees that "unlocker" will release the
> lock on the mutex.
>
> One issue that was brought up is the locking/unlocking of the mutex
> during stack unwinding after an exception is thrown. This
> implementation does not "solve" that problem. That problem is, in
> fact, unavoidable, and present in any scope-based locking scheme. The
> reason it is unavoidable is because inner scopes do not have knowledge
> of how outer scopes will handle exceptions, and outer scopes must be
> able to make correct assumptions about the state of the mutex when
> handling exceptions. Therefore, "optimizations" such as skipping a
> transient lock during stack unwinding are simply not possible without
> breaking assumptions about the mutex's state along the way.
That's right. The exception might cause a transient locks. But be sure
you will need that. Think of an application function, which does other
clean-up tasks during stack unwinding, assuming that it has the lock.
The same applies the other way around, when a clean-up relies on the
fact that the mutex is not owned.
> Here is the implementation I came up with. It defines the concept of
> locked and unlocked "regions". It uses an std::vector<int> to track
> the nesting level of regions. It also provides "unscoped" locking and
> unlocking for completeness. In the code below, a simple benchmark of
> these two loops:
Your implementation looks rather complicated to me.
If you have a function that reports some kind of thread ID there is a
much easier approach. Usually this function is available and cheap.
#include <assert.h>
// Mutex wrapper
class mutex
{public:
void lock() {} // *** TODO!!!
void unlock() {} // *** TODO!!!
};
// fetch the current thread ID
// It could return whatever it likes as long as it is biunique.
int get_tid();
// Recursive Mutex Wrapper
// Strictly speaking this class could be joined with class mutex
// for example by deriving from it.
class mutex_locker
{private:
mutex& M;
int TID;
unsigned Count;
public:
mutex_locker(mutex& m) : M(m), TID(0), Count(0) {}
~mutex_locker() { assert(!Count); }
void lock(unsigned count = 1)
{ assert(count);
int tid = get_tid();
// Reading TID is not synchronized, but the result of the comparsion
// is reliable as long as you have an aligned machine size int
// because of the mutex.
if (TID != tid)
{ M.lock();
TID = tid;
}
// We own the mutex. We can safely access Count.
Count += count;
}
void unlock(unsigned count = 1)
{ assert(count);
assert(TID == get_tid());
assert(count <= Count);
Count -= count;
if (Count == 0)
{ TID = 0;
M.unlock();
}
}
unsigned get_count() const
{ return TID == get_tid() ? Count : 0;
}
};
class scoped_lock
{private:
mutex_locker& L;
public:
scoped_lock(mutex_locker& l) : L(l) { L.lock(); }
~scoped_lock() { L.unlock(); }
};
class scoped_unlock
{private:
mutex_locker& L;
unsigned Count;
public:
scoped_unlock(mutex_locker& l)
: L(l),
Count(L.get_count())
{ if (Count) L.unlock(Count); }
~scoped_unlock() { if (Count) L.lock(Count); }
};
Marcel
JC
wrote:
Thanks for your reply.
Actually, the mutex_locker (in my implementation) is only intended to
be used by a single thread (although more than one thread may have a
mutex_locker wrapping the same mutex, more than one thread won't share
a mutex_locker). The counter / tracking info only tracks locking
information for that one thread.
The reason the implementation is so complicated is because it provides
more than just recursive support. It provides one extra feature,
namely a guarantee that a scoped_unlock *will* unlock the mutex lock
even if it is nested inside multiple scoped_locks:
mutex_locker locker(g_mutex);
{
scoped_lock lock(locker); // <-- lock acquired
{
scoped_lock lock(locker); // <-- no-op
{
scoped_unlock unlock(locker); // <-- lock released
// mutex is UNLOCKED at this point.
} // <-- lock acquired
} // <-- no-op
} // <-- lock released
It provides the same guarantee for scoped_locks nested in multiple
scoped_unlocks. A pure count-based implementation would require a
scoped_unlock to match each scoped_lock before the mutex was actually
unlocked, e.g., with your example:
mutex_locker locker(g_mutex);
{
scoped_lock lock(locker); // <-- acquired (++count)
{
scoped_lock lock(locker); // <-- no-op (++count)
{
scoped_unlock unlock(locker); // <-- no-op (--count)
// <-- mutex is still LOCKED!
{
scoped_unlock unlock(locker); // <-- released (--count==0)
// <-- now mutex is unlocked.
} // <-- acquired (++count)
} // <-- no-op (++count)
} // <-- no-op (--count)
} // <-- released (--count==0)
The extra complexity with the vector provides this functionality by
tracking how many levels into similar "regions" you are nested, and
then acquiring/releasing the lock when you transition between locked
and unlocked regions regardless of the nesting "balance".
There is a comment in the source that describes this, but basically
each element of the vector is a count of consecutive entrances to a
given region type (locked or unlocked), with positive integers
representing a locked region and negative integers representing
unlocked. So the vector:
3 -2 4 -1
Means that at the current position, you have locked the mutex 3 times,
then unlocked it 2 times, then locked it 4 times, then unlocked it 1
time (via the mutex_locker), and the mutex is currently *unlocked*.
Note that the vector always starts with a positive number, alternates
signedness, and never contains a 0 (these are invariants of
mutex_locker member functions).
When entering a locked region, if the last entry is negative, you know
you are transitioning from unlocked -> locked and must acquire the
mutex. A new entry, 1, is added. If the last entry is positive, it is
simply incremented but no action is taken on the mutex. The same is
true for entering an unlocked region, but with opposite signs. Note
that when entering an unlocked region, if the vector is *empty*, it
will cause an assertion failure, because it means you did something
like this:
mutex_locker locker(g_mutex);
{
scoped_unlock unlock(locker); // never locked.
}
That assertion could be removed but extra logic would have to be added
to support that unless the underlying mutex has the ability to Do The
Right Thing when it is released after never having been acquired.
So, the complexity over a simple count-based implementation is
necessary because of the nesting guarantees, it's more than just a
recursive wrapper around non-recursive mutexes.
Jason
David Schwartz
> The specific problem this addresses is nested scoped locks and
> unlocks. For example, in the following situation:
>
> {
> scoped_lock locker(g_mutex);
> {
> scoped_lock locker(g_mutex);
> {
> scoped_unlock unlocker(g_mutex);
> // <-- if g_mutex supports recursive locks, then
> // it actually remains locked here.
> }
> }
> }
I think you misunderstand the nature of the problem. The *reason* the
mutex is locked is because the caller expects no intervening threads
to access the object. By releasing the mutex even though the code that
placed the outer lock didn't ask you to, you violate the assumptions
that outer code made.
> The implementation below guarantees that "unlocker" will release the
> lock on the mutex.
Thereby breaking the assumptions the code that acquired the first lock
made. If the code that acquired the first lock wanted to transfer its
lock to the code that acquired the second lock, it could have done so.
If the code that acquired the second lock was allowed to release the
first lock, that's what would have been done.
Consider:
mutex.Lock();
Foo();
Func1();
Func2();
mutex.Unlock();
If 'Func1' can only be called by code that already holds this lock, it
would never try to acquire its own lock and this problem would never
happen. Therefore 'Func1' can be called with or without a lock. Now,
why would this chunk of code choose to call 'Func1' *with* a lock?
There's only one answer -- it expects the object to remain stable
across the call to 'Func1'.
By 'cleverly' releasing the lock inside Func1, you defeat the entire
point of the caller acquiring the lock in the first place.
So you fix the breakage inside Func1 by breaking its callers. I don't
see how that helps.
DS
Marcel Müller
JC wrote:
> Thanks for your reply.
>
> Actually, the mutex_locker (in my implementation) is only intended to
> be used by a single thread (although more than one thread may have a
> mutex_locker wrapping the same mutex, more than one thread won't share
> a mutex_locker). The counter / tracking info only tracks locking
> information for that one thread.
In this case at least the assertions of my implementation will not hit
the nail on the head. But beyond that it will still work with thread
local mutex_locker instances, since no two threads can have a nonzero
lock count anyway.
If you really ensure that a mutex_locker instance is not used by more
than one thread, you may remove the TID field and all access to it from
my code. It will still work. But then you MUST not use it with different
threads. This is risky because you cannot easily make the objects
thread-local in a way that the compiler detects incorrect useage.
> The reason the implementation is so complicated is because it provides
> more than just recursive support. It provides one extra feature,
> namely a guarantee that a scoped_unlock *will* unlock the mutex lock
> even if it is nested inside multiple scoped_locks:
>
> mutex_locker locker(g_mutex);
> {
> scoped_lock lock(locker); // <-- lock acquired
> {
> scoped_lock lock(locker); // <-- no-op
> {
> scoped_unlock unlock(locker); // <-- lock released
> // mutex is UNLOCKED at this point.
> } // <-- lock acquired
> } // <-- no-op
> } // <-- lock released
This is exactly what happens with my implementation too.
> It provides the same guarantee for scoped_locks nested in multiple
> scoped_unlocks. A pure count-based implementation would require a
> scoped_unlock to match each scoped_lock before the mutex was actually
> unlocked, e.g., with your example:
>
> mutex_locker locker(g_mutex);
> {
> scoped_lock lock(locker); // <-- acquired (++count)
> {
> scoped_lock lock(locker); // <-- no-op (++count)
> {
> scoped_unlock unlock(locker); // <-- no-op (--count)
> // <-- mutex is still LOCKED!
No. Count will be decremented by 2 in this case and the mutex is released.
> {
> scoped_unlock unlock(locker); // <-- released (--count==0)
This would be a no-op.
> // <-- now mutex is unlocked.
> } // <-- acquired (++count)
no-op
> } // <-- no-op (++count)
aquire, count += 2
> } // <-- no-op (--count)
> } // <-- released (--count==0)
> The extra complexity with the vector provides this functionality by
> tracking how many levels into similar "regions" you are nested, and
> then acquiring/releasing the lock when you transition between locked
> and unlocked regions regardless of the nesting "balance".
>
> There is a comment in the source that describes this, but basically
> each element of the vector is a count of consecutive entrances to a
> given region type (locked or unlocked), with positive integers
> representing a locked region and negative integers representing
> unlocked. So the vector:
>
> 3 -2 4 -1
>
> Means that at the current position, you have locked the mutex 3 times,
> then unlocked it 2 times, then locked it 4 times, then unlocked it 1
> time (via the mutex_locker), and the mutex is currently *unlocked*.
> Note that the vector always starts with a positive number, alternates
> signedness, and never contains a 0 (these are invariants of
> mutex_locker member functions).
>
> When entering a locked region, if the last entry is negative, you know
> you are transitioning from unlocked -> locked and must acquire the
> mutex. A new entry, 1, is added. If the last entry is positive, it is
> simply incremented but no action is taken on the mutex. The same is
> true for entering an unlocked region, but with opposite signs.
Feel free to do so, but I think the complexity is not required to
satisfy your needs.
> So, the complexity over a simple count-based implementation is
> necessary because of the nesting guarantees, it's more than just a
> recursive wrapper around non-recursive mutexes.
I don't think so. The complete information in the vector is not needed
together at any place in your code. Only the deepmost element counts. So
why not distribute the information over the objects in the callstack.
This is exactly what I have done. The mutex_locker only keeps track of
the last state. In case of unlocks even the unlock count is insignificant.
Furthermore I have the following recommendations:
- Have a look at conditional variables, if they fulfill your needs in a
way that you do not have to deal with explicit unlocks at all. This is a
cleaner application design in my opinion. (They operate similar to my
implementation.) If conditinal variables are no choice then
- implement the core logic to deal with the unlocking in your mutex
class and throw away the thread local mutex_locker. This will make
things easier, because you never can accidentially operate on a
mutex_locker of a different thread.
- The mutex class should always have a public function the check the
locked state for the current thread. While this might not be needed in
the main code lines, it is very helpful for assertions at the start of
function bodies to verify the preconditions 'mutex must be locked' or
'mutex must be unlocked'.
- Do not use unscoped locks or unlocks at all, unless you really want to
ensure to have some bugs in your application. Times have changed with
C++ and it is nearly impossible to get the unscoped locks exception safe
on a sustained basis.
You may protect the mutex::lock and unlock functions and declare the
scoped_lock/unlock classes as friends of mutex instead. This ensures a
high degree of code consistency.
- If you really want to be able to lock or unlock the mutex at a certain
code location, extend the scoped_lock class by an explicit unlock and
lock function and a locked flag. Example:
class scoped_lock
{private:
mutex_locker& L;
bool Own;
public:
scoped_lock(mutex_locker& l) : L(l), Own(true) { L.lock(); }
~scoped_lock() { if (Own) L.unlock(); }
unlock() { assert(Own); L.unlock(); Own = false; }
lock() { assert(!Own); L.lock(); Own = true; }
};
Than you will be able to release a lock earlier e.g. in case of an error
exit, that should not write the log message to the file system within
locked context. However, if you have a nested lock, that won't help you
and even a scoped_unlock will not be safe, since the caller cannot be
sure about the state of the variables protected the lock when the callee
unexpectedly released the lock in case of an error.
- Strictly speaking, once you need a recursive lock, your code is not
that clean. It is because this breaks the assumption of a callee, that
outside the locked region safely other things can be done. This
sometimes results in deadlocks, when function calls outside the locked
region might imply other mutex locks, which are no longer deadlock-safe
if the callee is called inside a nested lock. Believe, I made the
experience - the hard way.
Marcel
JC
days.
On Dec 31 2008, 9:48 pm, Marcel Müller <news.5.ma...@spamgourmet.com>
wrote:
> > The reason the implementation is so complicated is because it provides
> > more than just recursive support. It provides one extra feature,
> > namely a guarantee that a scoped_unlock *will* unlock the mutex lock
> > even if it is nested inside multiple scoped_locks:
>
> > mutex_locker locker(g_mutex);
> > {
> > scoped_lock lock(locker); // <-- lock acquired
> > {
> > scoped_lock lock(locker); // <-- no-op
> > {
> > scoped_unlock unlock(locker); // <-- lock released
> > // mutex is UNLOCKED at this point.
> > } // <-- lock acquired
> > } // <-- no-op
> > } // <-- lock released
>
> This is exactly what happens with my implementation too.
You are right, I think. It took a while for me to convince myself.
Yours seems to meet all the requirements and is definitely a lot
simpler. I didn't think of it that way at all.
Your advice here is all really good, too:
> Furthermore I have the following recommendations:
>
> - Have a look at conditional variables, if they fulfill your needs in a
> way that you do not have to deal with explicit unlocks at all. This is a
> cleaner application design in my opinion. (They operate similar to my
> implementation.) If conditinal variables are no choice then
>
> - implement the core logic to deal with the unlocking in your mutex
> class and throw away the thread local mutex_locker. This will make
> things easier, because you never can accidentially operate on a
> mutex_locker of a different thread.
>
> - The mutex class should always have a public function the check the
> locked state for the current thread. While this might not be needed in
> the main code lines, it is very helpful for assertions at the start of
> function bodies to verify the preconditions 'mutex must be locked' or
> 'mutex must be unlocked'.
Good point.
> - Do not use unscoped locks or unlocks at all, unless you really want to
> ensure to have some bugs in your application. Times have changed with
> C++ and it is nearly impossible to get the unscoped locks exception safe
> on a sustained basis.
> You may protect the mutex::lock and unlock functions and declare the
> scoped_lock/unlock classes as friends of mutex instead. This ensures a
> high degree of code consistency.
The unscoped stuff was more for "completeness", so I thought, but you
are, of course, right. I did protect mutex_locker's functions, but
moving that logic into mutex would allow it to happen there instead,
although it seems like it would be nice to have just a plain old mutex
available for situations where you know you don't need the extra
features (that's why I didn't protect mutex's functions and make
mutex_locker a friend).
> - If you really want to be able to lock or unlock the mutex at a certain
> code location, extend the scoped_lock class by an explicit unlock and
> lock function and a locked flag. Example:
> class scoped_lock
> {private:
> mutex_locker& L;
> bool Own;
> public:
> scoped_lock(mutex_locker& l) : L(l), Own(true) { L.lock(); }
> ~scoped_lock() { if (Own) L.unlock(); }
> unlock() { assert(Own); L.unlock(); Own = false; }
> lock() { assert(!Own); L.lock(); Own = true; }
> };
> Than you will be able to release a lock earlier e.g. in case of an error
> exit, that should not write the log message to the file system within
> locked context. However, if you have a nested lock, that won't help you
> and even a scoped_unlock will not be safe, since the caller cannot be
> sure about the state of the variables protected the lock when the callee
> unexpectedly released the lock in case of an error.
This is the approach that I'd normally use, to be honest. The
scoped_unlock was the result of a discussion in another thread, in
practice I've never been in a situation where I needed it, and when I
do need to release a scoped lock early, the above method is precisely
how I do it -- generally I design the code explicitly to avoid nested
locks so that's not an issue. It is a much better solution, you are
right.
> - Strictly speaking, once you need a recursive lock, your code is not
> that clean. It is because this breaks the assumption of a callee, that
> outside the locked region safely other things can be done.
Agreed. I don't actually need the scoped_unlock I described, it was
more of an exercise (that it looks like I didn't do too well at).
> This
> sometimes results in deadlocks, when function calls outside the locked
> region might imply other mutex locks, which are no longer deadlock-safe
> if the callee is called inside a nested lock. Believe, I made the
> experience - the hard way.
Thanks for the great reply,
Jason
JC
> On Dec 31, 2:13 am, JC <jason.cipri...@gmail.com> wrote:
>
> > The specific problem this addresses is nested scoped locks and
> > unlocks. For example, in the following situation:
>
> > {
> > scoped_lock locker(g_mutex);
> > {
> > scoped_lock locker(g_mutex);
> > {
> > scoped_unlock unlocker(g_mutex);
> > // <-- if g_mutex supports recursive locks, then
> > // it actually remains locked here.
> > }
> > }
> > }
>
> I think you misunderstand the nature of the problem. The *reason* the
> mutex is locked is because the caller expects no intervening threads
> to access the object. By releasing the mutex even though the code that
> placed the outer lock didn't ask you to, you violate the assumptions
> that outer code made.
I probably twisted the problem a bit now that you mention it. The
original reason was a solution to the problem (that I think you
presented, but I might be misremembering) "what if I want to modify
the code in a scoped_lock to release the lock in the middle
somewhere?" In the discussion, the problem eventually transformed to
"what if I want to unlock a mutex that the caller has locked and I'm
not sure of the nesting level?" The difference between the two being
that the former is a controlled scenario where you know exactly what
you want, and the latter has a lot more unknowns. The presented
solution to the former really isn't appropriate for the latter, I
guess.
> > The implementation below guarantees that "unlocker" will release the
> > lock on the mutex.
>
> Thereby breaking the assumptions the code that acquired the first lock
> made. If the code that acquired the first lock wanted to transfer its
> lock to the code that acquired the second lock, it could have done so.
> If the code that acquired the second lock was allowed to release the
> first lock, that's what would have been done.
Yes, you are right. In the original problem, the code that acquired
the first lock didn't make any assumptions because it was known that
it was supposed to be unlocked in the middle.
> Consider:
> mutex.Lock();
> Foo();
> Func1();
> Func2();
> mutex.Unlock();
>
> If 'Func1' can only be called by code that already holds this lock, it
> would never try to acquire its own lock and this problem would never
> happen. Therefore 'Func1' can be called with or without a lock. Now,
> why would this chunk of code choose to call 'Func1' *with* a lock?
> There's only one answer -- it expects the object to remain stable
> across the call to 'Func1'.
>
> By 'cleverly' releasing the lock inside Func1, you defeat the entire
> point of the caller acquiring the lock in the first place.
>
> So you fix the breakage inside Func1 by breaking its callers. I don't
> see how that helps.
If it is known by the caller that Func1() will release the lock, then
it is not so much of a problem. But in that case, a cleaner solution
may be one of the ones Marcel Müller suggested -- adding an explicit
unlock/lock to the scoped_lock and having the caller pass it's lock to
Func1().
Jason
David Schwartz
> I probably twisted the problem a bit now that you mention it. The
> original reason was a solution to the problem (that I think you
> presented, but I might be misremembering) "what if I want to modify
> the code in a scoped_lock to release the lock in the middle
> somewhere?"
Correct. In this case, I (the currently-running scope) acquired the
lock and I want to release it, it's my lock.
> In the discussion, the problem eventually transformed to
> "what if I want to unlock a mutex that the caller has locked and I'm
> not sure of the nesting level?"
I've never heard of that problem and it doesn't make sense to me.
> If it is known by the caller that Func1() will release the lock, then
> it is not so much of a problem. But in that case, a cleaner solution
> may be one of the ones Marcel Müller suggested -- adding an explicit
> unlock/lock to the scoped_lock and having the caller pass it's lock to
> Func1().
And if the caller passes its lock to a function, it's quite clear that
the recipient may release that lock.
It is almost never rational to have a scope or function that can be
called with or without a lock that, if called with that lock, release
it. At minimum, such a case should have a parameter to indicate
whether it's being called with or without the lock, so that the caller
knows the lock is being messed with when it chooses the parameters.
DS