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

[Chris Hegarty]

Thanks Alan, David, Mandy for you comments.

 > The retry loop in checkThreadState make sense. Is the 100ms sleep a
 > bit excessive? The thread will likely get to the expected state in a
 > fraction of that time.

True, reduced to 10ms.

 > 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.

 > L130-116: I agree with David that this test is not needed.
 > Like the RuntimeException listed in ProblemList.txt shows
 > that the target thread is in WAITING state but expected to
 > be RUNNABLE.

The main thread executes goBlocked, goWaiting, goTimedWaiting, etc. 
These methods set the state and then wait for the phaser to advance. The 
phaser will not advance until MyThread triggers it, at which point the 
thread should either be RUNNABLE or the expected state, right? Or have I 
missed something?

I added this extra check since we are now relaxing the check for the 
expected state. I just thought it would be more correct than allowing 
any state, but if you feel it too strict ( or still incorrect ) I can 
remove it.

 > 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.

 > L98: Are you seeing some timing issue if you keep this
 > checkThreadState
 >     before the join and thus you moved it after the join?

No timing issue. I did this for simplicity, given that the state should 
be TERMINATED when join returns. Either way MyThread.run must complete 
before the threads state will be set to TERMINATED. Invoking 
checkThreadState before that point just seems more likely encounter 
retries. I'm ok with either, just let me know if you want it reverted 
back to the original.

 > 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 ...

When MyThread invokes arriveAndAwaitAdvance, then it should be the final 
party to arrive and therefore "probably" shouldn't wait. But you raise a 
good point, myThread should really just invoke phaser.arrive() rather 
that arriveAndAwaitAdvance.

Updated Webrev:
   http://cr.openjdk.java.net/~chegar/7021010/jdk8.webrev.01/webrev/

-Chris.


On 06/20/11 01:54 PM, Chris Hegarty wrote:
> 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/
>
> -Chris.