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

[Chris Hegarty]

On 06/21/11 12:12 PM, Chris Hegarty wrote:
> 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.

Oh, I also increased the max retries to 500 since reducing the sleep 
time. This is just a precaution for busy systems.

-Chris.


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