Re: [boost] [Thread][Release] Boost.Thread and 1.45 release

Subject: Re: [boost] [Thread][Release] Boost.Thread and 1.45 release
From: vicente.botet (vicente.botet_at_[hidden])
Date: 2010-10-29 08:30:26

----- Original Message ----- 
From: "Anthony Williams" <anthony.ajw_at_[hidden]>
To: <boost_at_[hidden]>
Sent: Friday, October 29, 2010 8:52 AM
Subject: Re: [boost] [Thread][Release] Boost.Thread and 1.45 release

> 
> "vicente.botet" <vicente.botet_at_[hidden]> writes:
> 
>> From: "Anthony Williams" <anthony.ajw_at_[hidden]>
> 
>>> I've fixed all the outstanding "showstopper" bugs in boost.thread on
>>> trunk (including the thread interruption issue, #2330), 
> 
>> I have take a look at the modification associated to this ticket #2330
>> thread::interrupt() can be lost if condition_variable::wait() in
>> progress and I don't reach to understand why do you need to have an
>> internal mutex on condition_variable. The associated patch don't
>> modifies this point. The resolution don't states nothing more than the
>> ticket has been solved but no how. Please could you explain this
>> addition? Is this related to another ticket?
> 
> The patch attached to the ticket is incomplete. There is no requirement
> that the same mutex is used with the same condition variable instance
> for every wait. Therefore, if a thread waits with one mutex, wakes,
> destroys the mutex and then waits with another then there is a race
> condition with another thread that tries to interrupt it --- the
> interrupting thread might try and lock a just-destroyed mutex.

Oh, I see. So to support interruptions, the condition variable needs to be associated to a single mutex which must be locked before any condition variable wait. 
I don't understand however why do you need to lock the mutex each time you signal the condition

    inline void condition_variable::notify_one()
    {
***        boost::pthread::pthread_mutex_scoped_lock internal_lock(&internal_mutex);
        BOOST_VERIFY(!pthread_cond_signal(&cond));
    }
        
    inline void condition_variable::notify_all()
    {
***        boost::pthread::pthread_mutex_scoped_lock internal_lock(&internal_mutex);
        BOOST_VERIFY(!pthread_cond_broadcast(&cond));
    }

    void thread::interrupt()
    {
        detail::thread_data_ptr const local_thread_info=(get_thread_info)();
        if(local_thread_info)
        {
            lock_guard<mutex> lk(local_thread_info->data_mutex);
            local_thread_info->interrupt_requested=true;
            if(local_thread_info->current_cond)
            {
***                boost::pthread::pthread_mutex_scoped_lock internal_lock(local_thread_info->cond_mutex);
                BOOST_VERIFY(!pthread_cond_broadcast(local_thread_info->current_cond));
            }
        }
    }

Please, could you clarify what are you protecting with the lock (***)?

I was wondering why do we need to pass a unique_lock to the wait operations if it is not needed at all? 
 
> The internal mutex is necessary to eliminate this race condition. This
> mutex could be global, but that would introduce a point of contention
> between all cond vars.

 
>> This will make condition_variable and condition_variable_any almost
>> equivalent. What will be the adavantage to use condition_variable
>> respect to condition_variable_any?
> 
> The only difference now is that condition_variable_any will accept any
> type of mutex in the wait functions. There is no difference between
> them in terms of performance.

If I remember the restriction on condition_variable was to be able to be as efficient as if we used the underlying platform. If we don't have any advantage for condition_variable, why to have it? As not all applications use the interruption mechanism, I will preserv a condition_variable class that is not an interruption point and is as efficient as possible.
 
Best,
Vicente