Re: [Boost-users] Probably bug in condition::timed_wait()

From: Moshe Matitya (Moshe.Matitya_at_[hidden])
Date: 2006-09-20 09:39:35

Peter Dimov wrote:
> 
> Pavel Syomin wrote:
> > Hi,
> >
> > I'm porting custom library from UNIX to Windows and found that some
> > piece of code work correctly on UNIX and some time segfaulting  on
> > Windows. I think, that problem is on boost::condition::timed_wait()
> > function. To be sure I write small test:
> 
> [...]
> 
> You are assuming that when condition::(timed_)wait returns 
> the predicate (in 
> your case m_counter != 0) is guaranteed to be true. This is 
> not correct. 
> condition::wait is allowed to return at any time (this is known as a 
> "spurious wakeup".) You need to use a while loop.
> 
> while( m_counter == 0 )
> {
>     if( !m_condition.timed_wait( lock, time ) ) return; // timeout
> }

Alternatively, you can avoid the need to explicitly use a while loop by
calling the overloaded version of timed_wait() that takes a predicate
argument.  This version does the loop for you internally.  E.g.:

        if (!(m_condition.timed_wait (lock, time, boost::bind<int>
                (boost::mem_fn (&AsyncQueue::m_counter), this))))
            return;

On a separate point, there is a bug in your usage of the xtime struct in
main():

        xtime_get(&time, TIME_UTC);
        time.nsec += 10000;
        queue.timed_pop(time);

You cannot simply add a value to time.nsec and then use the struct,
because nsec is required to be in the range [0,999999999].  [See
<http://www.cl.cam.ac.uk/~mgk25/time/c/>.]  By adding a value to nsec,
you may cause it to overflow, which would invalidate the struct, and
possibly cause unexpected behavior.  You need to normalize the xtime
struct by "carrying over" any excess part of nsec to sec.  E.g.:

#define NSECS_PER_SEC   1000000000
...
        xtime_get(&time, TIME_UTC);
        time.nsec += 10000;
        if (time.nsec >= NSECS_PER_SEC)
        {
            time.sec  += time.nsec / NSECS_PER_SEC;
            time.nsec  = time.nsec % NSECS_PER_SEC;
        }
        queue.timed_pop(time);

(This is an ugly but necessary consequence of Boost.Thread's unfortunate
design decision to use a raw struct to represent time values.  It would
have been much better to use a class that encapsulates its internals,
and that knows how to normalize itself.)

Moshe