Race condition in ThreadGroup stop test

[David Holmes]

Gary,

I agree with Remi - the initialization process can be simplified as 
shown. As long as first is created and started first it should do the 
job. You should test that the updated test still fails on a JDK without 
the fix.

I'm a little concerned about the change to wait/notify due to the fact 
the notifyAll happens while the ThreadDeath is pending. But this should 
either work or not work, I don't think it will introduce further obscure 
races (though with Thread.stop I don't take anything for granted).

David

On 9/11/2011 1:54 AM, Rémi Forax wrote:
> 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 }
>>
>