Race condition in ThreadGroup stop test

[Rémi Forax]

On 11/08/2011 02:12 PM, Gary Adams wrote:
>  Let's see if this captures all the comments and
> is a bit more robust in the face of the original
> race conditions mentioned.
>     - threads are started before they are recorded in local variables
>     - second is now volatile
>     - stop thread group is triggered by first thread once second 
> thread is started
>     - main thread checks for second thread death after thread group is 
> stopped
>     - left in the sleep delays to yield to other threads
>     - using wait/notify for handshake with main thread
>     - using try/finally to ensure main thread does not check too soon
>       after thread group stop

I really don't like the thread.start() in the middle of the constructor.
In fact, we don't really need to initialize first and second in the 
constructor of Stop.

I propose:
public class Stop implements Runnable {
   private static boolean groupStopped =false ;
   private static final Object lock = new Object();

   private static final Thread first;
   private static final Thread second;
   static {
      ThreadGroup group = new ThreadGroup("");
      first = new Thread(group, new Stop());
      second = new Thread(group, new Stop());
   }

In that case, the is no constructor in Stop because
the default one is Ok.

with this initialization sequence, first and second are never null,
so the check line 59 is useless.

And in main(), the two threads are started like this
public static void main(String[] args) throws Exception {
     // Launch two threads as part of the same thread group
     first.start();
     second.start();
     ...


I find this code far more readable.

cheers,
Rémi

>
>     31    public class Stop implements Runnable {
>     32        private static Thread first=null;
>     33        private static volatile Thread second=null;
>     34        private static ThreadGroup group = new ThreadGroup("");
>     35        private static boolean groupStopped =false ;
>     36        private static final  Object lock = new Object();
>     37
>     38        Stop() {
>     39            Thread thread = new Thread(group, this);
>     40            thread.start();
>     41            // Record the threads after they have been started
>     42            if (first == null)
>     43                first = thread;
>     44            else
>     45                second = thread;
>     46        }
>     47
>     48        public void run() {
>     49            while (true) {
>     50                try {
>     51                    // Give the other thread a chance to start
>     52                    Thread.sleep(1000);
>     53                } catch(InterruptedException e) {
>     54                }
>     55
>     56                // When the first thread sees the second thread 
> has been started
>     57                // stop the thread group.
>     58                if ((Thread.currentThread() == first)
>     59 && (second != null)) {
>     60                    synchronized (lock) {
>     61                        try {
>     62                            group.stop();
>     63                        } finally {
>     64                            // Signal the main thread it is time 
> to check
>     65                            // that the stopped thread group was 
> successful
>     66                            groupStopped = true;
>     67                            lock.notifyAll();
>     68                        }
>     69                    }
>     70                }
>     71            }
>     72        }
>     73
>     74        public static void main(String[] args) throws Exception {
>     75            // Launch two threads as part of the same thread group
>     76            for (int i = 0; i < 2; i++)
>     77                new Stop();
>     78
>     79            // Wait for the thread group to be issued
>     80            synchronized(lock){
>     81                while (!groupStopped){
>     82                    lock.wait();
>     83                    try {
>     84                        // Give the other thread a chance to stop
>     85                        Thread.sleep(1000);
>     86                    } catch(InterruptedException e) {
>     87                    }
>     88                }
>     89            }
>     90
>     91            // Check that the second thread is terminated when the
>     92            // first thread terminates the thread group.
>     93            boolean failed = second.isAlive();
>     94
>     95            // Clean up any threads that may have not been 
> terminated
>     96            first.stop();
>     97            second.stop();
>     98            if (failed)
>     99                throw new RuntimeException("Failure.");
>    100        }
>    101    }
>