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

From: vicente.botet (vicente.botet_at_[hidden])
Date: 2008-05-08 07:43:29

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

> "vicente.botet" <vicente.botet_at_[hidden]> writes:
>
>> ----- Original Message ----- 
>> From: "Anthony Williams" <anthony_at_[hidden]>
>> To: <threads-devel_at_[hidden]>
>> Sent: Monday, April 14, 2008 5:34 PM
>> Subject: Re: [Threads-devel] [boost] Expected behaviour of condition
>> variable wait when unique_lock is not locked
>>
>>
>>> 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.
>>
>> "void wait(unique_lock<mutex>& lock);
>> Precondition:
>>     lock is locked by the current thread, and either:
>>     No other thread is waiting on this condition_variable object, or
>>     The lock arguments supplied by all concurrently waiting threads (via
>> wait or timed_wait) return the same value for lock.mutex().
>> Effects:
>>     Atomically calls lock.unlock() and blocks on *this.
>>     When unblocked, calls lock.lock() (possibly blocking on the lock) and
>> returns.
>>     The function will unblock when this thread is signaled by a call to
>> this->notify_one(), a call to this->notify_all(), or spuriously.
>>     If the function exits via an exception, lock.lock() will still be 
>> called
>> prior to exiting the function scope.
>> Postconditions:
>>     lock is locked by the current thread.
>> Throws:
>>     system_error when the effects or postconditions cannot be achieved.
>> "
>
> 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.

>From 
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2406.html#condition_rationale
"Below is an example implementation of cond_var on top of pthread_cond_t. 
The reference implementation is meant to demonstrate how thinly cond_var 
maps to pthread_cond_t (or whatever native OS condition variable is 
available).
class cond_var
{
    pthread_cond_t cv_;
public:

    cond_var()
    {
        error_code::value_type ec = pthread_cond_init(&cv_, 0);
        if (ec)
            throw system_error(ec, native_category, "cond_var constructor 
failed");
    }

<snip>
    void wait(unique_lock<mutex>& lock)
    {
        error_code::value_type ec = pthread_cond_wait(&cv_, 
lock.mutex()->native_handle());
        if (ec)
            throw system_error(ec, native_category, "cond_var wait failed");
    }
"

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?

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
    }

>> 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?
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()));
}

> condition_variable_any
> will support any lock type that has unlock() and lock() operations.

condition_variable_any is another history. From 
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2406.html#condition_rationale

"
I have experimented with templating the condition variable but have 
discovered problems with this approach.
* If the condition is templated on lock type, then the wait functions are 
not templated. This destroys the ability to simultaneously wait on a 
unique_lock<shared_mutex> and a shared_lock<shared_mutex> on the same 
shared_mutex.
* If the condition is templated on mutex type, then the wait functions can 
be templated on lock type, solving the previous problem. However one is 
still depending on a specialization of this condition to provide the razor 
thin layer over the OS condition variable (e.g. pthread_cond_t). That 
specialization can not reliably have its wait functions templated on lock 
type. Such a lock would be required to do nothing but lock/unlock the mutex, 
which would outlaw user defined lock types such as the locked file example 
mentioned in the mutex rationale. The specialization must only allow waiting 
on a standard lock type (i.e. unique_lock<mutex>).

Because the condition specialized on the native mutex type can not have the 
same interface as the primary condition template (it can't wait on any lock 
type), specialization is not appropriate for this application (reference the 
vector<bool> example).

The only conclusion I can come to which supports both a razor thin layer 
over the native OS condition variable, and a generalized condition variable 
which works with user defined mutexes/locks (such as the Lock2 example) is 
two distinct types:
* cond_var: A condition variable that can wait on nothing but 
unique_lock<mutex> (or perhaps mutex).
* gen_cond_var: A condition variable that can wait on anything which 
supports lock() and unlock().
"

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.

template <typename Lockable>
class strict_lock : private boost::noncopyable /*< Is not copyable >*/ {
    BOOST_CONCEPT_ASSERT((LockableConcept<Lockable>));
public:
    typedef Lockable lockable_type;
    explicit strict_lock(lockable_type& obj)
        : obj_(obj) { obj.lock(); } /*< locks on construction >*/
    ~strict_lock() { obj_.unlock(); } /*< unlocks on destruction >*/

    typedef bool (strict_lock::*bool_type)() const; /*< safe bool idiom >*/
    operator bool_type() const { return &strict_locker::owns_lock; }
    bool operator!() const { return false; } /*< always owned >*/
    bool owns_lock() const { return true; }

    bool is_locking(lockable_type* l) const { return l==mutex(); } /*<
strict lockers specific function >*/
    /*< no possibility to unlock other than with conditional variables>*/
private:
    lockable_type& obj_;
    strict_lock(); /*< disable default constructor >*/
    BOOST_NON_ALIAS(strict_lock); /*< disable aliasing >*/
    BOOST_NON_HEAP_ALLOCATED(strict_lock); /*< disable heap allocation >*/

    friend class boost::condition_variable;
    friend class boost::condition_variable_any;
    lockable_type* mutex() { return &obj_; }
    void lock() {obj_.lock();}
    void unlock() {obj_.unlock();}

};

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.

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

template<typename Mutex>
class lock_guard
{
private:
    Mutex& m;
    explicit lock_guard(lock_guard&);
    lock_guard& operator=(lock_guard&);
    // new
    friend class boost::condition_variable_any;
    friend class boost::condition_variable;
    Mutex* mutex() { return &m; }
    void lock() {m.lock();}
    void unlock() {m.unlock();}

public:
    explicit lock_guard(Mutex& m_):
    m(m_)
    {
        m.lock();
    }
    lock_guard(Mutex& m_,adopt_lock_t):
        m(m_)
    {}
    ~lock_guard()
    {
        m.unlock();
    }
    // new
    bool is_locking(Mutex* l) const
    {
        return l==mutex(); }
    };

With this lock_guard, is there any deep raison to don't have:

void condition_variable::wait(lock_guard<mutex>& m);

Best

Vicente