Re: [boost] [thread 1.48] Multiple interrupt/timed_join leads to deadlock

Subject: Re: [boost] [thread 1.48] Multiple interrupt/timed_join leads to deadlock
From: Vicente J. Botet Escriba (vicente.botet_at_[hidden])
Date: 2012-12-07 14:53:37

Le 07/12/12 20:26, Gaetano Mendola a écrit :
> On 07/12/2012 19.03, Vicente J. Botet Escriba wrote:
>> Le 07/12/12 01:23, Gaetano Mendola a écrit :
>>>
>>> Even the implementation can be improved, even if it's thread safe it
>>> doesn't avoid to issue a join to an already joined thread, or for the
>>> matter to interrupt a thread already joined/interrupted.
>>>
>> I don't know if you comment has something to be with these two tickets
>>
>> https://svn.boost.org/trac/boost/ticket/7668: thread_group::join_all()
>> should check whether its threads are joinable
>
> Intersting, I saw the fix and as you can see it has a race condition 
> there, indeed a thread can detach itself between the call to joinable 
> and the join at that point even if the check was made when the join is 
> issued the precondition doesn't hold anymore.
>
> Basically that is the question I'm asking: how can we be sure that a
> thread is joinable when we call the join on it, for sure checking the
> "joinable" has some problems. I have solved this in my thread group
> implementation with a flag saying if the thread was already
> joined or not and this works only because our threads do not detach
> them self so the thread group is the only responsible for the status
> change.
>
 From my point of view a thread in a thread_group is owned by the thread 
group. Any direct operation on a thread belonging to the thread group 
falls in undefined behavior. From the documentation

Member function create_thread()

template<typename F>
thread* create_thread(F threadfunc);

Effects:

     Create a new boost::thread object as-if by new thread(threadfunc) and add it to the group.
Postcondition:

     this->size() is increased by one, the new thread is running.
Returns:

     A pointer to the new boost::thread object.

Member function add_thread()

void add_thread(thread* thrd);

Precondition:

     The expression delete thrd is well-formed and will not result in undefined behaviour.
Effects:

     Take ownership of the boost::thread object pointed to by thrd and add it to the group.
Postcondition:

     this->size() is increased by one.

>> https://svn.boost.org/trac/boost/ticket/7669: thread_group::join_all()
>> should catch resource_deadlock_would_occur
>
> That is another story about the exception safety of thread_group, and
> as you can see it's not exception safe.
Please, let me know where there is an issue and I'll try to fix it.
>
>>> I also believe the create_thread returning a boost::thread pointers is
>>> very dangerous, indeed doing so the boost::thread_group doesn't have
>>> anymore the ownership of it, it would be better if create_thread would
>>> have returned a thread id instead.
>> I don't want to invest to much on the thread_group class, as for me this
>> class was designed before threads were movable and a complete redesign
>> should be done. You can of course create a Trac ticket requesting this
>> new feature.
>
> Due the limitation of this class I have wrote my own thread group with
> also the method  join_any having the semantic to interrupt all threads
> if any of the threads terminate. I believe seeing the current status of
> thread_group class the best to do is or fix it for good or deprecate it.
I'm for deprecating it, as I posted already sometime ago, but of course 
I'm for fixing the bugs I could be introduced by recent modification in 
other parts of the library. Anyway, before deprecating it, we need an 
alternative. Maybe you have already a concrete proposal and you can 
submit it to Boost.

Best,
Vicente