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

From: vicente.botet (vicente.botet_at_[hidden])
Date: 2008-05-08 10:34:10

----- Original Message ----- 
From: "Anthony Williams" <anthony_w.geo_at_[hidden]>
To: <boost_at_[hidden]>
Sent: Thursday, May 08, 2008 3:33 PM
Subject: Re: [boost] [thread] Expected behaviour of condition 
variablewaitwhen the mutex is not locked

> "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.

Thanks. I have missed this point.

>> 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.

Do you mean that it will useful to do this 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).

OK.

>> 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.

Yes we can as explained below, if this function is private and the 
condition_variable is a friend class.

>> 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.
>
>>> 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.

Thanks

Vicente