Code Review 7021010: java/lang/Thread/ThreadStateTest.java fails intermittently
chris.hegarty at oracle.com
Tue Jun 21 11:39:04 UTC 2011
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.
> > 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:
> 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.
More information about the core-libs-dev