Race condition in ThreadGroup stop test

[Brian Goetz]

Wait/notify may be better than sleeping, but semaphore/latch/barrier are much better than wait/notify.  Much.  

Sent from my iPhone

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