Re: [boost] [thread] Expected behaviour of condition variable waitwhen the mutex is not locked

From: Anthony Williams (anthony_w.geo_at_[hidden])
Date: 2008-05-08 09:33:01

"vicente.botet" <vicente.botet_at_[hidden]> writes:

> From: "Anthony Williams" <anthony_w.geo_at_[hidden]>

>> "vicente.botet" <vicente.botet_at_[hidden]> writes:
>>
>>> From: "Anthony Williams" <anthony_at_[hidden]>
>>>> Quoting "vicente.botet" <vicente.botet_at_[hidden]>:
>>>>
>>>>> which is the expected behaviour of condition variable wait when
>>>>> unique_lock is not locked
>>>>
>>>> Undefined behaviour: a precondition has not been met.
>>>
>>> IMO this is not satisfactory. At least an exception should be thrown. And
>>> the C++0x recomendation states this.

>> Then you're reading it differently to how it was written. The precondition 
>> is
>> "lock is locked by the current thread". If the precondition is violated, 
>> all
>> bets are off.
>
> You are right, as usual. It's the reference implementation described in 
> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2406.html#condition_rationale 
> which induced me on error.
>
> What do you think to add the raison you have prefered to abort the program 
> instead of throwing an exception on the rationale of your documentation?

I think it's a logic error in the application, so should be detected as soon
as possible during testing. You can always change it to throw by using the
"throw on assert" option for Boost.Assert.

> The following wait implementation works with models of strict locks. The 
> user is able to add the runtime checks adding the corresponding defines. 
> This implementation do not suffer from undefined behaviour when:
> * Different mutexes were supplied for concurrent wait() or timedwait() 
> operations on the same condition variable.
> * The mutex was not owned by the current thread at the time of the call.
>
> But
> * needs to store the reference to the mutex on the condition_variable. When 
> the user do not defines them the behaviour is equivalent.
> * Strict Lock must provide in addition
>     - owns_lock
>     - is_locking, and
>     - specialize is_strict_lock
>
> What is wrong on this approach?
>
>
>     template <class Locker>
>     void wait(Locker& l) {
>         BOOST_CONCEPT_ASSERT((StrictLockConcept<Locker>));
>         BOOST_STATIC_ASSERT((is_strict_lock<Locker>::value));
>         /*< Locker is a strict lock "sur parolle" >*/
>         BOOST_STATIC_ASSERT((is_same<
>             Lockable,
>             typename lockable_type<Locker>::type>::value));
>         /*< that locks the same lockable type >*/
> #      ifndef BOOST_THREAD_CONDITION_VARIABLE_DONT_CHECK_OWNERSHIP
>         /*< define BOOST_THREAD_CONDITION_VARIABLE_NO_CHECK_OWNERSHIP
>             if you don't want to check locker ownership >*/
>         if (! l) throw lock_error(); /*< run time check throw if no locked
>  >*/
> #    endif
> #    ifndef BOOST_THREAD_CONDITION_VARIABLE_DONT_CHECK_SAME
>         /*< define BOOST_THREAD_CONDITION_VARIABLE_DONT_CHECK_SAME
>             if you don't want to check locker check the same lockable >*/
>         if (!l.is_locking(&m)) throw lock_error(); /*< run time check throw
> if not locks the same >*/
> #    endif
>       // ... do as for unique_lock
>     }

You could do those checks with unique_lock.

>>> I'm wondering why the condition_variable wait operation do not requires a
>>> strict lock (the one introduced by Andrei Alexandrescu in his article 
>>> about
>>> external locking "Multithreading and the C++ Type System") instead of a
>>> unique lock or whatever.
>>
>> The wait must unlock the mutex and then lock it again. By using 
>> unique_lock
>> (which supports that), this possibility is explicit.
>
>>From the Thread documentation:
> "
> Effects:
> Atomically call lock.unlock() and blocks the current thread. The thread will 
> unblock when notified by a call to this->notify_one() or this->notify_all(), 
> or spuriously. When the thread is unblocked (for whatever reason), the lock 
> is reacquired by invoking lock.lock() before the call to wait returns. The 
> lock is also reacquired by invoking lock.lock() if the function exits with 
> an exception.
> "
>
> Here it is your implementation
>
> inline void condition_variable::wait(unique_lock<mutex>& m)
> {
>     detail::interruption_checker check_for_interruption(&cond);
>     BOOST_VERIFY(!pthread_cond_wait(&cond,m.mutex()->native_handle()));
> }
>
> I don't see any call to m.unlock()/m.lock(). Should you change the Effects 
> section?

No. That's the pthread version, where the OS does the unlock/lock. The Windows
version has to do that explicitly. Since this version only takes
unique_lock<mutex>, it can rely on knowing what the implementation of
unique_lock<mutex> looks like for optimization (as in this implementation).

> Why we can not have:
>
> inline void condition_variable::wait(strict_lock<mutex>& m)
> {
>     detail::interruption_checker check_for_interruption(&cond);
>     BOOST_VERIFY(!pthread_cond_wait(&cond,m.mutex()->native_handle()));
> }

If your strict lock exposes the mutex with a .mutex() member function, you
can't guarantee strictness.

> I'm not asking to have a single condition_variable, neither asking to add
>     void condition_variable_any::wait(strict_lock<mutex>& m)
>
> We can have a strict_lock which define this lock/unlock operations private 
> and declares friend condition_variable_any.

Yes, you could.

> Evidently this strict_lock will not be locked while waiting on a condition 
> variable and the operation own_lock will be erroneus. But this operation can 
> not be called because the thread is waiting on the condition. Note that the 
> current implementation of condition_variable::wait(unique_lock<mutex>& m) 
> produce the same symptomes on the unique_lock.

True.

>>
>> Boost 1.34 and prior used hidden traits types to unlock and relock the 
>> mutex,
>> which I don't think is desirable.
>>
>> The Boost 1.35 "strict lock" is boost::lock_guard.
> The raison we can not use a lock_guard is that it not provides the mutex 
> operation. It will be enough that lock_guard declares mutex as private 
> operation and make condition_variable a friend class.
>
> Next follows some additions to lock_guard that without changing the public 
> interface, allows to use it with a condition_variable_any.

> With this lock_guard, is there any deep raison to don't have:
>
> void condition_variable::wait(lock_guard<mutex>& m);

I guess not. I'll think about it.

Anthony
-- 
Anthony Williams            | Just Software Solutions Ltd
Custom Software Development | http://www.justsoftwaresolutions.co.uk
Registered in England, Company Number 5478976.
Registered Office: 15 Carrallack Mews, St Just, Cornwall, TR19 7UL