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

[David Holmes]

Mandy Chung said the following on 06/21/11 20:08:
> On 6/20/11 8: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/
> 
> 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.
> 
> L98: Are you seeing some timing issue if you keep this checkThreadState
>      before the join and thus you moved it after the join?
> 
> As to David's comment about the use of Phaser that delays the thread
> from making the state change.  The test was modified to use
> ThreadExecutionSynchronizer  as a fix to:
>    5039275 ThreadBlockedCount jtreg test fails on Solaris sparc on 
> service_sdk nightly
> 
> ThreadExecutionSynchronizer.signal() actually awaits until the
> main thread calls waitForSignal.  If I understand correctly, it's
> essentially doing the same thing as Phaser.arriveAndAwaitAdvance().
> So Chris' change to use Phaser only follows the existing code.

Thanks Mandy I missed that detail.

David
-----


> I thinkThreadExecutionSynchronizer was initially created to fix
> ThreadBlockCount testthat can make sure the thread enters the BLOCKED
> state an exact number of times and applied to other tests as it
> yields tighter timing window but might not be necessary.
> 
> Anyway, like David said, it isn't incorrect. If you don't have
> time to eliminate the need of the main thread to loop, I'm fine
> with the change to use Phaser but worths tuning the sleep time in L117.
> 
> Thanks
> Mandy
> 
> 
> 
> 
>