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-11 15:23:13

Le 11/12/12 19:12, Gaetano Mendola a écrit :
> On 11/12/2012 16.13, Vicente Botet wrote:
>> Gaetano Mendola-3 wrote
>>>
>>> 1) Thread group is now thread safe, it can be used concurrently by
>>>      multiple threads
>>
>> Why a thread group should be inherently thread-safe? It seems to me that
>> having a thread container is already useful.
>
> It can manage without pestering the developers the fact that one 
> entity spawns a batch of threads and then wait for the completion 
> waiting on
> the join() while another entity (an user interface as example) can 
> stop the whole process if it's taking too much time. Otherwise as soon
> someone performs a boost::thread_group::join then nothing can be done
> from outside to stop the process. It seems a natural use to me.
OK I think I understand your use case. Here it is an alternative that 
don't use any mutex to protect the group of thread.
I will choose and owner of all these threads, insert them on a 
container. Only this thread is able to join/interrupt the threads.
I will use some way to transfer the request from the user interface 
thread to the owner that this is taking too much time (using atomic?).
The owner will try to join each thread using try_join_until with the 
desired expiration time. If the thread is joined the thread is removed 
from the container. If there is a timeout the owner will check the 
protected state 'take_too_much_time' and will interrupt all the other 
threads and then join all of them. As you can see the contention is reduced.

Note that this is a specific behavior that can not be added to the 
thread_group class. I will be for the addition of an algorithm/free 
function that try to join the threads on a container/range during a 
given duration or until an expiration time (removing the joined threads).
>>> 2) thread_group now maintains 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
>>
>> IMO, all the threads in a thread_group are owned by the group, and 
>> use move
>> semantics, no need to use pointer to threads. As a consequence there 
>> is no
>> need for the handler/wrapper.
>
> This is true if the thread_group does not permits to be used by multiple
> threads interrupting/joining.
I understand now why you did this way. But I will not do that.
>
>>> 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.
>>
>> Could you clarify your concern?
>
> Sure, if a thread performs a closed loop:
>
> a) lock_mutex;
> b) timed_join
> c) unlock_mutex
> d) goto a
>
> and another thread is doing:
>
> a) lock_mutex;
> b) interrupt
> c) unlock_mutex
>
> then it goes in starvationm we have observed this (even in a
> deterministic way), then I had to make the two interrupt/timed_join on
> the thread handler fair each other.
> Our platform is a linux platform with a 3.2.0 kernel.
>
IMO, only one thread should join/interrupt all the threads. This avoid 
all these issues.
>> Why you have needed to change the implementation of join_all? It 
>> seems more
>> complex now.
>
> Because if you want to issue an interrupt while someone is joining a
> thread you need to have a join timed in order to periodically release
> the mutex protecting the thread.
I see why now.
>
>> As I said I'm for deprecating thread_group in Boost.Thread. The
>> implementation you are proposing has not changed my mind.
>
> Fine with me, leaving it as it is now is worst than
The class thread_group is not a big one. You are free to propose to 
Boost whatever you consider is good for the Boost community.
>
>> I would prefer an approach where data structures (containers) and 
>> algorithms
>> (join, interrupt, ...) are separated, thread-safety is not mandatory, 
>> ...
>
> I agreed with you about the fact if not explicitly written a library is
> not meant to be thread safe, this rule doesn't fit a thread library.
This is your opinion and I respect it even if I don't share it.
> I suggest to explicitly write, even in bold, the fact that thread_group
> and thread classes are not thread safe.
I believed that we agreed that the thread group was thread-safe! Of 
course the thread class is not thread-safe. I could add something like 
all the functions in this library are not thread-safe until stated 
explicitly, as I did it for Boost.Chrono since the beginning.

Best,
Vicente