[Boost-users] Re: boost::thread_group design flaw

From: Michael Glassford (glassfordm_at_[hidden])
Date: 2004-03-17 10:46:13

Pete wrote:
> I'm pretty convinced that there is a serious design flaw in
> boost::thread_group or at the very minimum, the documentation should
> better address the issue.

[example snipped]

> This example works fine until I make the following small change to
> main() which breaks it:
>
> int main(int argc, char* argv[])
> {
>     boost::thread_group threads;
>     for (int j = 0; j < 2; ++i)
>     {
>       for (int i = 0; i < 10; ++i)
>           threads.create_thread(&increment_count);
>       threads.join_all();
>     }
> }
>
> The above code will break the second time threads.join_all() is
> called. The problem is that all boost::thread objects created in the
> first iteration survive the join_all() call.

A thread_group is documented as destroying added thread objects when
its own destructor is called (see
http://boost.org/libs/thread/doc/thread.html#class-thread_group-synopsis).
Your example should probably be changed to:

int main(int argc, char* argv[])
{
    for (int j = 0; j < 2; ++i)
    {
      boost::thread_group threads; //MOVED THIS LINE
      for (int i = 0; i < 10; ++i)
          threads.create_thread(&increment_count);
      threads.join_all();
    }
}

> Worse still, the 2nd
> join_all() call sends another join() message to each thread it
> created but boost::thread::join() does not check the m_joinable flag

This is dealt with in the version of Boost.Thread which is in the
thread_dev branch of cvs, which I'm slowly moving into the main
branch.

> and simply tries to free the system resources it previously freed
> again causing a system fault.
> To correct the problem above, I can change the code as follows:
>
> int main(int argc, char* argv[])
> {
>     boost::thread*          pt[10];
>     boost::thread_group threads;
>     for (int j = 0; j < 2; ++i)
>     {
>       for (int i = 0; i < 10; ++i)
>           pt[i] = threads.create_thread(&increment_count);
>       threads.join_all();
>       for (int i = 0; i < 10; ++i)
>           threads.remove_thread(pt[i]);
>     }
> }
>
> While this code no longer causes a GPF, I do not believe that the
> boost::thread objects created by
boost::thread_group::create_thread()
> actually ever get destroyed.

Looking at the code, this appears to be correct: the thread* is
removed from the internal list but is never deleted. If you remove the
thread from the group, you are responsible for deleting it.

> I have set a break point in
> boost::thread::~thread() but this breakpoint is never reached. If I
> physically delete each pt[i] after removing it from the thread_group
> I'm getting an assertion error telling me that the pointer I'm
trying
> to delete is not a valid debug-heap pointer, but this error will
most
> likely disappear if I stop using the debug heap (I haven't tried
yet).
>
> As a boost::thread_group user, I made the following assumptions:
>
> - a boost::thread created through boost::thread_group::create_thread
> will be removed and cleaned up properly when a) the thread
terminates

This, as you discovered, is false.

> or b) the thread_group is deleted

This is true.

> - after returning from a call to join_all() the boost::thread_group
> has no boost::thread object members

This, as you discovered, is also false.

> - I need not specifically keep track of boost::thread objects
created
> by boost::thread_group and destruction of these objects is
> encapsulated within boost::thread_group

This is true, but only in the thread_group destructor.

It would probably be a good idea for the documentation to make all of
these issues clearer, as you suggest at the top of your post.

> I now find that none of these assumptions is quite correct. Perhaps
I
> good immediate remedy would be to implement a
> boost::thread_group::clear() method which would throw an error if
any
> of the threads where still in a running state and else would clear
> its threads collection and destroy the objects?

This is probably a good idea.

> How about thread
> objects "remembering" their membership in a thread_group so that the
> thread group can be informed when a thread terminates?