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 }
