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: Gaetano Mendola (mendola_at_[hidden])
Date: 2012-12-11 05:46:16

On 07/12/2012 22.24, Vicente J. Botet Escriba wrote:
> Le 07/12/12 21:19, Gaetano Mendola a écrit :
>> On 07/12/2012 20.53, Vicente J. Botet Escriba wrote:
>>> 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.
>>
>> I do full agree, that's why the thread_group::create_thread has to
>> return void or at most the thread id.
> yes a thread::id would be a better option.
>
>> What can the user do with a pointer to the create thread? All members
>> are not safe to be called apart two static functions.
> You can remove a thread from a thread_group :)
>> I believe not returning the thread pointer (that can not be touched
>> anyway) would improve thread_group interface.
>>
>> > 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.
>>
>> I should rework our thread group to adapt it to new 1.52 interface.
> Great. It will be a pleasure to review it.

Attached my first proposal, I don't know if this is the correct way
to submit it. I have named it Thread_Group (capitalized) to not conflict 
with old thread_group implementation.

1) Thread group is now thread safe, it can be used concurrently by
    multiple threads

2) thread_group now mantains a list of handlers with the responsibility
    to:
      -) Avoid join and interrupts to be called concurrently on a thread
      -) Avoid to call join on a joined thread
      -) Avoid to call interrupt on a joined/interrupt thread

3) Added join_any with the semantic to wait of any of the threads to
    terminate.

3) create_thread now returns a thread::id
    *this is a break change*, what we can do now is to leave the
    create_thread as it is (returning a thread pointer) but returning
    not a real thread but a class inherited by thread allowing only
    the get_id method on it (other methods should be implemented with
    throw("method not callable").

4) Due the fact mutex are not fair a thread issuing an interrupt_all
    most likely will go in starvation if a thread is issuing a join_all
    (especialy if the group contains a single thread). I can work at
    it.

Regards
Gaetano Mendola