Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changing thread state metadata is not thread safe #224

Closed
brycelelbach opened this issue Jul 10, 2012 · 6 comments

Comments

Projects
None yet
2 participants
@brycelelbach
Copy link
Member

commented Jul 10, 2012

[reported by blelbach] [Trac time Sun Oct 23 22:49:24 2011] A threads thread_state_enum and thread_state_ex_enum are both stored in two
separate atomic objects. In threadmanager_impl<>::set_state, these two variables
are assigned independently of each other (e.g. without a third form of synchronization).
This operation is not thread safe; it is possible for another set_thread_state operation
to modify one or both of these values before the first call returns.

The set_thread_state unit test demonstrates this bug. This unit test creates one
target pxthread, which enters an infinite yield loop. Each time this thread is
woken up, it checks the thread_state_ex_enum code that it was woken with. If the
code is wait_signaled or wait_timeout, then it stores this information and yields
again. If the code is wait_terminate, then it prints out the results of the test,
and returns.

In addition to the target thread, a number of worker threads (here, 64) are created
in a recursive tree. Each worker thread performs two set_thread_state operations
in serial. First, it calls set_thread_state(target_thread, pending, wait_signaled).
Then, it calls set_thread_state(target_thread, suspended, wait_timeout).

If changing thread state is an atomic operation, then the target thread should never
be resumed with wait_timeout.

[22:24:38]:wash@vega:/home/wash/hpx/gcc-4.6.1-release$ bin/set_thread_state -t40 --futures=64
woken:     4/128
signaled:  3/64
timed out: 1/0
[22:24:39]:wash@vega:/home/wash/hpx/gcc-4.6.1-release$ bin/set_thread_state -t40 --futures=64
woken:     5/128
signaled:  4/64
timed out: 1/0

Note that the number of times actually woken to the number of times scheduled to
be woken is off because of a related but different bug.

@brycelelbach

This comment has been minimized.

Copy link
Member Author

commented Jul 10, 2012

[comment by blelbach] [Trac time Thu Oct 27 18:14:42 2011] r6128

@brycelelbach

This comment has been minimized.

Copy link
Member Author

commented Jul 10, 2012

[comment by blelbach] [Trac time Tue Feb 28 21:47:11 2012] Hartmut and I discussed closing this today, but upon review I think it's still valid.

@brycelelbach

This comment has been minimized.

Copy link
Member Author

commented Jul 10, 2012

[comment by blelbach] [Trac time Tue Feb 28 22:05:16 2012] This needs to be re-verified on the top of trunk

@brycelelbach

This comment has been minimized.

Copy link
Member Author

commented Aug 22, 2012

Verified by #428

@sithhell

This comment has been minimized.

Copy link
Member

commented Dec 23, 2012

I can't reproduce this anymore. Executing the test gives these results (repeatedly):

build/hpx/release:130:$ ./bin/set_thread_state_test -t12
woken:     12/128
signaled:  12/64
timed out: 0/0

Is this still valid?

@sithhell

This comment has been minimized.

Copy link
Member

commented Jan 14, 2014

Can we close this now?

@hkaiser hkaiser changed the title Changing pxthread state metadata is not thread safe Changing thread state metadata is not thread safe Apr 17, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.