Race condition in ThreadGroup stop test

[Chris Hegarty]

Wow Gary, this is much better ( no thanks to my comments ;-) )

Nearly there now, please bear with us. I'll try again to be constructive 
(but you may need to wait for David to come back online to confirm my 
comments below :-) )

1) 'first' also needs to be volatile since it is set in one thread and 
accessed in another (outside of a sync block, or without any barrier).

2) I wouldn't just swallow the InterruptedException. jtreg uses thread 
interrupts to kill/interrupt a test that has used more than it's 
allotted time. I think it best to just throw them up the stack where 
possible, and wrap them in an Error or RuntimeException in run(). But 
either way allow the thread to be interruptible and complete execution.

Thanks again for taking this issue,
-Chris.

On 11/ 8/11 01: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
>
> 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 }
>