Re: [boost] [chrono] [thread] boost.chrono changes to boost::condition_variables

Subject: Re: [boost] [chrono] [thread] boost.chrono changes to boost::condition_variables
From: Morgan (morgan.henry_at_[hidden])
Date: 2010-07-23 10:18:40

Howard Hinnant <howard.hinnant <at> gmail.com> writes:
> 
> Fwiw, boost::chrono is modeled after std::chrono set to be standardized
> in C++0X:
> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2010/n3092.pdf
> 

Thanks.  That's useful guidance.  I'll assume wait_for and wait_until 
overloads using std::chrono will become standard at some future point.

That leads me to my next question - I believe the boost::condition_variable
implementation has a bug when dealing with the monotonic clock.  And the
existing and proposed boost::condition_variable API prevent a bug free
implementation.

> > My ultimate goal is to provide an implementation of
> > boost::condition_variables that allows access to the 
> > POSIX clock CLOCK_MONOTONIC 
> > for use through boost::condition_variables::timed_wait().
> > Specifically, providing an implementation that uses 
> >    pthread_condattr_setclock(&attr, CLOCK_MONOTONIC) at
> > boost::condition_variables initialisation,
> > and use of 
> >    pthread_cond_timedwait(..., &t) with clock_gettime(CLOCK_MONOTONIC, &t)
> > in boost::condition_variables::timed_wait() and friends
> > This change would provide reasonable behaviour of
> > boost::condition_variables::time_wait() in presence of ongoing changes
> > to the system clock.  

Currently, the condition_variable implementation (existing and proposed) is
strongly bound to the system clock for waits with time-outs; i.e. the "system
clock" is used for timed waits irrespective of your desired time base.

All wait_duration/rel_time overloads always assume CLOCK_REALTIME.

        template<typename duration_type>
        bool condition_variable::timed_wait(unique_lock<mutex>& m,
             duration_type const& wait_duration)
        {
            return timed_wait(m,boost::get_system_time()+wait_duration);
        }

        template <class Rep, class Period>
        bool condition_variable::wait_for(unique_lock<mutex>& lock, 
             const chrono::duration<Rep, Period>& rel_time)
        {
            return (timed_wait(lock,
                    convert_to<posix_time::time_duration>(rel_time))
        }

Even if you supply a chono::monotonic_clock::time_point (CLOCK_MONOTONIC) it is
erroneously converted to a CLOCK_REALTIME.

        template <class Clock, class Duration>

        bool wait_until(unique_lock<mutex>& lock, const
             chrono::time_point<Clock, Duration>& abs_time)
        {
            return timed_wait(lock, convert_to<system_time>(abs_time), pred);
        }

        bool condition_variable::timed_wait(unique_lock<mutex>& m,
             boost::system_time const& wait_until)
        
The implementation _always_ assumes the POSIX CLOCK_REALTIME time base before
forwarding its calculations to pthread_cond_timedwait.  This creates bugs when
you wish the condition_variable to operate in eg the CLOCK_MONOTONIC time base
by using chono::monotonic_clock.

In my view, the new boost::condition_variable API is incomplete, and lacks the
ability to properly bind the condvar to one of the new clock types.  Ideally,
you would want time calculations to remain in the time base you selected. ie, it
should use <clock_type>::now() for relative time calculations.  (To do this,
I've had to supply an additional clock_type template parameter.) 

       template<typename clock_type, typename duration_type>
       bool condition_variable::wait_for(unique_lock<mutex>& m, 
                   const clock_type &clock, const duration_type &rel_time)
       {
           return wait_until(m, clock_type::now() + rel_time);
       }

       bool condition_variable::wait_until(unique_lock<mutex>& m,
                   chrono::monotonic_clock::time_point const& wait_until)

       bool condition_variable::wait_until(unique_lock<mutex>& m,
                   chrono::system_clock::time_point const& wait_until)

Critically, this arrangement honours the required clock type, and allows the
time parameter to be passed through to pthread_cond_timedwait(..., &t) 
unmolested.

While the boost::condition_variable API remained ignorant of clocks other than
"the system clock" this problem was just an inconvienience.  However, now that
condition variables have acquired the explicit capability to reference the POSIX
CLOCK_MONOTONIC (through the monotonic_clock and time_point<monotonic_clock>
classes), the API is _actively_ misleading.

As for a solution, I think boost::condition_variables should become
parameterised on the clock type - mixing 2 time bases in a single condition
variable is broken. 

        template<clock_type>
        class condition_variable_clocked;
        // backwards compatibility
        typedef boost::condition_variable<system_clock> condition_variable;

        // create a condvar that uses POSIX CLOCK_MONOTONIC
        boost::condition_variable<chrono::monotonic_clock> cv1; 

        // create a condvar that uses POSIX CLOCK_REALTIME
        boost::condition_variable<chrono::system_clock> cv2;    

I'd appreciate any comments on the above.
regards
Morgan