Re: [boost] [threads] adopt_lock_t question

Subject: Re: [boost] [threads] adopt_lock_t question
From: Anthony Williams (anthony.ajw_at_[hidden])
Date: 2008-11-03 03:00:08

Jens Finkhäuser <jens_at_[hidden]> writes:

> Hi!
>
>   I've come across something that puzzles me. I've got code that,
> condensed to relevant parts, looks like this:
>
>     recursive_mutex m;
>     lock_guard<recursive_mutex> l1(m);
>     lock_guard<recursive_mutex> l2(m, adopt_lock_t());

That is an error. adopt_lock_t means that l2 takes ownership of the
already-locked mutex (i.e. doesn't lock it again), but it doesn't
force l1 to relinquish ownership, so both l2 and l1 think they own the
one and only lock. When l2 is destroyed it will unlock the mutex, and
then when l1 is destroyed it will unlock the mutex AGAIN, which is
undefined behaviour because it doesn't own the mutex at this point.

>   What happens is that if -DNDEBUG is set, the code passes. But if
> -DDEBUG is set, l1's destructor aborts. Further investigation shows
> that the destructor calls the mutexe's unlock() function (as
> expected), which in turn tries to unlock it's internal mutex object -
> in my case a pthread mutex.

That makes sense: in debug mode the unlock function detects the error.

>   As I understand the documentation, my code is perfectly valid. 

Unfortunately you are wrong.

> It
> may not make a large amount of sense to use adopt_lock_t as the nature
> of recursive_mutex should be to not block in l2's constructor - but
> that's a different matter.

That is true.

>   Now according to the documentation, lock_guard expects a Lockable,
> recursive_mutex is a Lockable, and Lockable's unlock() specifies very
> clearly what sort of conditions need to be met for the function to
> work:
>
>     Precondition:
>       The current thread owns *this.
>     Effects:
>       Releases ownership by the current thread.
>     Postcondition:
>       The current thread no longer owns *this.
>     Throws:
>       Nothing
>
>   So there you have it, in my case the current thread does not own
> *this after l2's destructor ran - in fact, no thread does.

Yes. The problem is that l1's destructor then *also* calls unlock.

>   Now there are a two inconsistencies here:
>
>   1. The code works fine with other Lockable implementations, such as
>      non-recursive boost mutexes.

That's just a fluke of undefined behaviour: sometimes it works how
you thought it would.

>   2. The behaviour is different depending on compilation mode. Yes,
>      that's the idea behind asserts, but parts of the documentation
>      suggest one behaviour is to be expected, other parts suggest the
>      other is.
>
>   From that I must draw the conclusion that either recursive_mutex and
> the unlock() documentation is broken, or the non-recursive mutexes
> are.

Nope. Your usage of lock_guard is broken.

>   Now conceptually, I'd expect unlock() to always succeed. That is, if
> it's postcondition can be fulfilled, it should always, regardless of
> compilation mode, exit cleanly. In that case the precondition docs
> should read something like "*this is either owned by the current
> thread, or by no thread at all.".

No. Detecting whether the current thread owns the mutex adds
unnecessary overhead. The win32 implementation of boost::mutex *does
not know* which thread owns the mutex.

Anthony
-- 
Anthony Williams
Author of C++ Concurrency in Action | http://www.manning.com/williams
Custom Software Development | http://www.justsoftwaresolutions.co.uk
Just Software Solutions Ltd, Registered in England, Company Number 5478976.
Registered Office: 15 Carrallack Mews, St Just, Cornwall, TR19 7UL, UK