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 13:12:35

On 11/12/2012 16.13, Vicente Botet wrote:
> Gaetano Mendola-3 wrote
>> 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:
>>
>>>>> 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.
>
> It depends on whether you want to submit an independent library. This has
> the advantage that you don't
> have to be backward compatible.
>
>
>> 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
>
> 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.

>> 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.

>> 3) Added join_any with the semantic to wait of any of the threads to
>>      terminate.
>
> I don't like the implementation polling on all the threads to see if one of
> them has ended.
> Maybe the use of at_thread_exit could help. If all the threads of a thread
> group are created with the create_thread function, an alternative is to use
> a decorator of the user function that can signal when the thread has been
> finished. This behavior is quite close to futures.

I agree with it.

>> 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").
>
> Do you really need to remove threads?
> I will even go for don't returning anything?
>
> I will remove also the add_thread function.

I agree as well.

>> 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.

> 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.

> 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

> 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.
I suggest to explicitly write, even in bold, the fact that thread_group
and thread classes are not thread safe.

Regards
Gaetano Mendola