Race condition in ThreadGroup stop test

[chris hegarty]

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