boost::thread_group design flaw
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.
Let's take a look at the thread_group.cpp example provided with
boost::thread:
#include
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.
'thread_group' is totally and utterly flawed and should never have been added to Boost.Threads in the first place. Use vector< shared_ptr<thread> >.
Peter Dimov wrote:
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.
'thread_group' is totally and utterly flawed and should never have been added to Boost.Threads in the first place. Use vector< shared_ptr<thread> >.
This is essentially what thread_group becomes in the Boost.Thread version that's on the thread_dev branch. In that version, the thread class becomes a handle class; each thread object contains a reference-counted pointer to a thread_data object. Only one thread_data object exists per thread, stored in a thread_specific_ptr (which, as an aside, is why the thread_specific_ptr class becomes more important, which is presumeably why Bill Kemp removed support for the Win32 static library version of Boost.Thread). When a thread object is created, it gets the associated thread_data object (or creates one if there isn't one already) and increments its reference count; when it detaches, it decrements the reference count. When the thread_data object is no longer referenced (which only happens when all thread objects and the thread_specific_ptr stop referencing it), the thread_data pointer is deleted and the thread is detached. At least that's what I see in the code; I have yet to try it to see how it works. Mike
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.
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
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. 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?
participants (3)
-
Michael Glassford -
Pete -
Peter Dimov