Code Review 7021010: java/lang/Thread/ThreadStateTest.java fails intermittently

[David Holmes]

Hi Chris,

Chris Hegarty said the following on 06/20/11 22:54:
> java/lang/Thread/ThreadStateTest.java can fail with when 
> checkThreadState finds an unexpected state.
> 
> Exception in thread "main" java.lang.RuntimeException: MyThread expected 
> to have TERMINATED but got RUNNABLE
>     at ThreadStateTest.checkThreadState(ThreadStateTest.java:119)
>     at ThreadStateTest.main(ThreadStateTest.java:96)
> 
> There is a race between the thread being put in a specific state and the 
> thread testing for that state. The test should retry the thread state 
> check a number of times before failing. Also, some minor cleanup and 
> update to use a more recent j.u.c reusable synchronization barrier.
> 
> http://cr.openjdk.java.net/~chegar/7021010/jdk8.webrev.00/webrev/

I'm not sure the extra check in checkThreadState that the thread must be 
RUNNABLE is valid. What if you are transitioning the thread from a 
blocked to non-blocked state, you may still see it blocked on the first 
call to getState.

I also don't understand why you moved the terminated check to after the 
join() - if the thread is failing to terminate then the join(), which is 
untimed, will simply not return and the test will hang until timed-out 
by the harness.

I also don't think the use of the Phaser is appropriate here as you are 
actually delaying the thread from making the state change. In the 
original code the target thread signals the main thread that it is about 
to go to state X and continues to advance to state X (modulo preemption 
etc). But with the Phaser the target thread indicates it is about to go 
to state X and then waits for the main thread - consequently it is more 
likely that when the main thread calls checkThreadState that the target 
has not yet reached the desired state and so the main thread will have 
to loop. This isn't incorrect it just seems to me that in the "wrong" 
configuration the test may not take a lot longer in relative terms. 
Maybe the additional clarity is worth it though ...

Cheers,
David