Boost C++ Libraries: Ticket #4345: thread::id and joining problem with cascade of threads

attachment set

Tue, 15 Jun 2010 14:57:50 GMT

attachment → thread_bug_test.tar.bz2

anonymous — Tue, 15 Jun 2010 21:31:18 GMT

g_count is not protected against race conditions. You should use an atomic integer or use a mutext to protect g_count.

Steven Watanabe — Wed, 16 Jun 2010 03:25:46 GMT

This code is very hard to follow, but here's what's happening in detail:

An object of type B is created
- This creates a shared_ptr with a new A object
  - The new A object starts thread A
Thread B is created, and receives a copy of the B object, this copy is not destroyed until the last thread::id for this thread is gone.
Thread B runs to completion and is joined. The last copy of B has not yet been destroyed, so there is no attempt to join thread A.
Kaboom.

I don't think this should be considered a bug in the library. If you want to join another thread when a thread exits, join at the end of the thread function instead of in the destructor.

Wed, 16 Jun 2010 05:12:27 GMT

Replying to anonymous:

g_count is not protected against race conditions. You should use an atomic integer or use a mutext to protect g_count.

it's true, what you say - the same goes for g_started. i left it this way intentionally since i work on x86 where ++x on integer is atomic any way and i didn't want to complicate example code any more.

Wed, 16 Jun 2010 05:51:22 GMT

Replying to steven_watanabe:

This code is very hard to follow, but here's what's happening in detail:

this is the easy case - trust me. i've spent last 2 days tracking it down in "real" system we're developing. ;)

An object of type B is created
This creates a shared_ptr with a new A object
The new A object starts thread A
Thread B is created, and receives a copy of the B object, this copy is not destroyed until the last thread::id for this thread is gone.

this is the key - thread::id holds identifier of a thread as well as handle to it. it means that thread::id has in fact double responsibility, thus non-obvious behavior (id!=handle).

Thread B runs to completion and is joined. The last copy of B has not yet been destroyed, so there is no attempt to join thread A.
Kaboom.
I don't think this should be considered a bug in the library. If you want to join another thread when a thread exits, join at the end of the thread function instead of in the destructor.

technically this can be done, though i don't think it's a proper way - by doing so one must remember in each thread's operator()()'s implementation to explicitly join all owned threads, instead of doing it in error-prone, RAII-way (d-tor perform clean up - here: int+join).

a took a look in thread::id implementation - it holds shared_ptr to "thread's data" (i assume this is a handle to thread itself). my suggestion is to change it to weak_ptr, so that thread::id would become only "observer" of the thread, that can be compared, put into stream, etc... this would eliminate double responsibility of thread::id. alternatively only data required to identify thread should be stored in thread::id - ex.: name string.

Anthony Williams — Wed, 16 Jun 2010 07:33:07 GMT

Replying to bartek szurgot <bartosz.szurgot@…>:

Replying to anonymous:

g_count is not protected against race conditions. You should use an atomic integer or use a mutext to protect g_count.

it's true, what you say - the same goes for g_started. i left it this way intentionally since i work on x86 where ++x on integer is atomic any way and i didn't want to complicate example code any more.

++x is not atomic on integers, even on x86. See http://www.devx.com/cplus/Article/42725

Anthony Williams — Wed, 16 Jun 2010 07:35:10 GMT

I agree that the thread function should be destroyed when the thread is joined. I am not sure of the best way to handle this --- maybe weak_ptr is the way to go, maybe another way is preferable.

Wed, 16 Jun 2010 08:50:41 GMT

Replying to anthonyw:

Replying to bartek szurgot <bartosz.szurgot@…>:

Replying to anonymous:

g_count is not protected against race conditions. You should use an atomic integer or use a mutext to protect g_count.

it's true, what you say - the same goes for g_started. i left it this way intentionally since i work on x86 where ++x on integer is atomic any way and i didn't want to complicate example code any more.

++x is not atomic on integers, even on x86. See http://www.devx.com/cplus/Article/42725

i've read linked article and run tests, but got different results. since this discussion is a bit out of scope of this ticket, please contact me directly (bartosz dot szurgot at pwr dot wroc dot pl) so we can proceed with this topic.

cc set

viboes — Wed, 17 Nov 2010 21:44:15 GMT

cc viboes added

Please, could you give a try to week_ptr and see how the solution you propose behaves?

attachment set

anonymous — Mon, 22 Nov 2010 10:23:28 GMT

attachment → thread_bug_test.tar.2.bz2

attachment set

anonymous — Mon, 22 Nov 2010 10:23:49 GMT

attachment → patch_thread_id.diff.bz2

attachment set

anonymous — Mon, 22 Nov 2010 10:24:16 GMT

attachment → patch_cond_warn.diff.bz2

bartosz.szurgot@… — Mon, 22 Nov 2010 10:28:48 GMT

i've tested it with boost from svn (revision 66677) and created patch that changes shared_prt<> to weak_ptr<> in thread::id. it appears to work (i.e. solves the problem). i've also attached changed version of test-program (locks for ++ and -- operations).

when doing so i've also found warning on condition variable (unused variable when no assert is present). i've made separate patch for this as well.

owner, status, milestone changed; cc deleted

viboes — Sun, 04 Dec 2011 00:00:08 GMT

cc viboes removed
owner changed from Anthony Williams to viboes
status new → assigned
milestone Boost 1.43.0 → To Be Determined

viboes — Fri, 09 Dec 2011 17:38:08 GMT

I was wondering what prevent from making thread::id an opaque type around the thread::native_handle, Anthony?

keywords set

viboes — Sun, 11 Dec 2011 09:53:59 GMT

keywords thread::id added

viboes — Sat, 31 Dec 2011 15:09:08 GMT

See also #5173 Bug/design issue with boost::thread::id

viboes — Sat, 07 Apr 2012 15:44:45 GMT

I've attached a patch to #5173 that should solve this issue also. Please could you give it a try?

milestone changed

viboes — Sun, 08 Apr 2012 22:07:26 GMT

milestone To Be Determined → Boost 1.50.0

Committed in trunk r77838

Wed, 25 Apr 2012 16:11:28 GMT

i've checked r77838 - it appears that it does not fix the problem. using boost's trunk (r78191, as of writing these words) however appears to fix it, i.e. using provided test program it does join both threads, as it should.

viboes — Mon, 28 May 2012 15:30:11 GMT

Have you defined BOOST_THREAD_PROVIDES_BASIC_THREAD_ID or BOOST_THREAD_VERSION=2?

status changed; resolution set

viboes — Mon, 28 May 2012 15:31:46 GMT

status assigned → closed
resolution → fixed

Committed in release branch at [78543]

bartek.szurgot@… — Tue, 19 Jun 2012 14:05:34 GMT

when BOOST_THREAD_PROVIDES_BASIC_THREAD_ID is set in r77838 test app works fine. i see that on trunk (r79003 as of writing these words) is is enabled by default. this probably was the difference.