Race condition in ThreadGroup stop test

[Rémi Forax]

On 11/07/2011 09:50 PM, Brian Goetz wrote:
> Wait/notify may be better than sleeping, but semaphore/latch/barrier are much better than wait/notify.  Much.
>
> Sent from my iPhone

Yeah, APIs evolve, it seems I'm to old for that s... :)

Rémi

>
> On Nov 7, 2011, at 1:28 PM, chris hegarty<chris.hegarty at oracle.com>  wrote:
>
>> Hi Gary,
>>
>> Thanks for taking this bug.
>>
>> In similar tests I've preferred to use j.u.c.Paser/CountDownLatch to coordinate between threads. So, you could completely remove the sleep(1000) from the run method and use a Phaser.arriveAndAwaitAdvance(). This will guarantee both threads will reach a particular point before invoking stop on the group. That way, either thread can invoke stop and set the flag to prevent the other thread from invoking stop too ( if it were to get there! ).
>>
>> I agree with Rémi's comments about wait/notify. My comments (above) should work with well with his suggestion. Then no more nasty sleeps!
>>
>> Thanks,
>> -Chris.
>>
>> On 07/11/2011 16:03, Gary Adams wrote:
>>>
>>> Here's another test with race conditions built into the test
>>> that can be easily avoided
>>>
>>> CR#7084033 - TEST_BUG: test/java/lang/ThreadGroup/Stop.java fails
>>> intermittently
>>>
>>> There are at least two race conditions in the test as currently written.
>>> The test uses sleeps as a way to yield time to other threads to complete
>>> their
>>> tasks, but this may not be sufficient on slower machines.
>>>
>>> The first race condition is in the starting of the test threads.
>>> To ensure both threads are started, the run check for the first thread
>>> should
>>> also check that the second thread is not null.
>>>
>>> The second race condition in the main thread presumes that
>>> the group stop has been issued within 3000 milliseconds.
>>> A simple loop on the delay can be gated by a flag set after the group
>>> stop has been issued.
>>>
>>> diff --git a/test/java/lang/ThreadGroup/Stop.java
>>> b/test/java/lang/ThreadGroup/Stop.java
>>> --- a/test/java/lang/ThreadGroup/Stop.java
>>> +++ b/test/java/lang/ThreadGroup/Stop.java
>>> @@ -32,6 +32,7 @@
>>> private static Thread first=null;
>>> private static Thread second=null;
>>> private static ThreadGroup group = new ThreadGroup("");
>>> + private static boolean groupStopped =false ;
>>>
>>> Stop() {
>>> Thread thread = new Thread(group, this);
>>> @@ -47,8 +48,11 @@
>>> while (true) {
>>> try {
>>> Thread.sleep(1000); // Give other thread a chance to start
>>> - if (Thread.currentThread() == first)
>>> + if ((Thread.currentThread() == first)
>>> +&&  (second != null)) {
>>> + groupStopped = true;
>>> group.stop();
>>> + }
>>> } catch(InterruptedException e){
>>> }
>>> }
>>> @@ -57,7 +61,10 @@
>>> public static void main(String[] args) throws Exception {
>>> for (int i=0; i<2; i++)
>>> new Stop();
>>> + do {
>>> Thread.sleep(3000);
>>> + } while (!groupStopped) ;
>>> +
>>> boolean failed = second.isAlive();
>>> first.stop(); second.stop();
>>> if (failed)
>>>
>>>
>>>