Amy, sorry you have so many picky reviewers!
Here's how I might write it:
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.atomic.AtomicReference;
public class Stop {
private static final CountDownLatch ready = new CountDownLatch(1);
private static final ThreadGroup group = new ThreadGroup("");
private static final AtomicReference<Throwable> firstThrowable
= new AtomicReference<>();
private static final AtomicReference<Throwable> secondThrowable
= new AtomicReference<>();
private static final Thread second = new Thread(group, () -> {
ready.countDown();
try {
do {} while (true);
} catch (Throwable ex) {
secondThrowable.set(ex);
}
});
private static final Thread first = new Thread(group, () -> {
// Wait until "second" is started
try {
ready.await();
group.stop();
} catch (Throwable ex) {
firstThrowable.set(ex);
}
});
public static void main(String[] args) throws Exception {
// Launch two threads as part of the same thread group
first.start();
second.start();
// Both threads should terminate when the first thread
// terminates the thread group.
second.join();
first.join();
if (! (firstThrowable.get() instanceof ThreadDeath) ||
! (secondThrowable.get() instanceof ThreadDeath))
throw new AssertionError("should have thrown ThreadDeath");
// Test passed - if never get here the test times out and fails.
}
}
On Mon, Jul 11, 2016 at 10:28 PM, Amy Lu <amy...@oracle.com> wrote:
> Please help to review the updated webrev, getting rid of ThreadDeath and
> using join():
>
> http://cr.openjdk.java.net/~amlu/8132548/webrev.03/
>
> Thanks,
> Amy
>
>
> On 7/11/16 6:55 PM, David Holmes wrote:
>
>>
>>
>> On 11/07/2016 6:14 PM, Amy Lu wrote:
>>
>>> Thank you David, though the email crossed-by, I hope all the concerns
>>> have been resolved in the updated webrev:
>>> http://cr.openjdk.java.net/~amlu/8132548/webrev.02/
>>>
>>
>> Sorry but no, the changes are neither necessary nor sufficient. With the
>> new code the ThreadDeath can hit anytime after countDown(), so it can hit
>> before you enter the try block.
>>
>> I had rewrote the test with CountDownLatch and join(long millis). Also
>>> unlike the old version, test thread 'first' and 'second' do different
>>> things, and 'second' thread will keep alive for waiting to be killed by
>>> group.stop() from 'frist' thread in the same thread group. I think this
>>> could avoid the second call to second.stop() (group.stop()) issue.
>>>
>>
>> Simply using join() with no timeout and relying on the test framework to
>> kill the test allows you to do away with the second stop().
>>
>> David
>> -----
>>
>> The time LONG_DELAY_MS used in join(long millis) is an adjusted timeout:
>>>
>>> private static final long LONG_DELAY_MS = Utils.adjustTimeout(1000L);
>>> ...
>>>
>>> second.join(LONG_DELAY_MS);
>>> boolean failed = second.isAlive();
>>>
>>> This gives second thread a more "reasonable" time to die, with taking
>>> account of the test harness timeout.
>>> This almost same as second.join()// test pass or timeout, but with
>>> additional chance of killing all threads in a bad case.
>>>
>>> Thanks,
>>> Amy
>>>
>>> On 7/11/16 3:25 PM, David Holmes wrote:
>>>
>>>> Simplification ...
>>>>
>>>> On 11/07/2016 5:12 PM, David Holmes wrote:
>>>>
>>>>> Hi Amy,
>>>>>
>>>>> Thanks for tackling this.
>>>>>
>>>>> On 8/07/2016 4:01 PM, Amy Lu wrote:
>>>>>
>>>>>> Thank you Joe for your review.
>>>>>>
>>>>>> The intent is to give it more chance "for the thread group stop to be
>>>>>> issued", not to extend the whole test execution timeout.
>>>>>>
>>>>>> I updated the webrev to make this in a retry, limit to 5 times of
>>>>>> retry:
>>>>>> http://cr.openjdk.java.net/~amlu/8132548/webrev.01/
>>>>>>
>>>>>
>>>>> The retry serves no purpose here. groupStopped is being set to true and
>>>>> the waiting thread has been notified, so the loop will terminate after
>>>>> the first sleep. The failure happens when the main thread checks the
>>>>> isAlive() status of the second thread before the ThreadGroup.stop has
>>>>> actually had a chance to stop and terminate it - such that isAlive is
>>>>> now false. That is why I suggested waiting a bit longer by extending
>>>>> the
>>>>> sleep.
>>>>>
>>>>> I agree that making the test last at least 5 seconds is not ideal, but
>>>>> I
>>>>> didn't think it would be an issue in the big picture of testing.
>>>>>
>>>>> Ideally explicit "synchronization" is better than sleeps but that would
>>>>> again be missing the point with this test. We expect the thread to
>>>>> terminate, if it hasn't terminated in a "reasonable" time we consider
>>>>> the stop() to have failed and the test to fail. To that end we could
>>>>> remove the sleep altogether and change:
>>>>>
>>>>> boolean failed = second.isAlive();
>>>>>
>>>>> to
>>>>>
>>>>> try {
>>>>> second.join(1000);
>>>>> } catch (InterruptedException shouldNotHappen) {}
>>>>> boolean failed = second.isAlive();
>>>>>
>>>>> Now we use explicit event synchronization - great! But the test has the
>>>>> same failure issue now as it did with the sleep. Putting in a
>>>>> CountDownLatch would have exactly the same problem: we expect the
>>>>> second
>>>>> thread to signal the latch as it terminates, but if that doesn't happen
>>>>> within a "reasonable" amount of time, then we deem the stop() to have
>>>>> failed and the test to have failed.
>>>>>
>>>>> Also note that the more code we add the more likely the second call to
>>>>> second.stop() triggers an async exception in code that can't handle it
>>>>> and results in an even worse failure mode!
>>>>>
>>>>> The only thing I can suggest is to get rid of the explicit sleep (or
>>>>> join, or latch.await() etc) and simply recode as an infinite loop and
>>>>> rely on the test harness to tear things down when the overall test
>>>>> timeout kicks in. That way the test either passes or is timed-out,
>>>>> there
>>>>> is no explicit failure - but a busy loop is also bad so you would want
>>>>> a
>>>>> small sleep in there anyway:
>>>>>
>>>>> while (second.isAlive()) {
>>>>> Thread.sleep(10);
>>>>> }
>>>>> // Test passed - if we never get here the test times out and
>>>>> // we implicitly fail.
>>>>>
>>>>
>>>> Of course that was silly all you need is:
>>>>
>>>> second.join();
>>>> // Test passed - if we never get here the test times out and
>>>> // we implicitly fail.
>>>>
>>>> David
>>>>
>>>>
>>>> Thanks,
>>>>> David
>>>>>
>>>>> Thanks,
>>>>>> Amy
>>>>>>
>>>>>> On 7/8/16 12:15 PM, joe darcy wrote:
>>>>>>
>>>>>>> Hi Amy,
>>>>>>>
>>>>>>> I'm a bit uncomfortable with the fix as-is.
>>>>>>>
>>>>>>> Rather than hard-coding sleep values, if sleep values are needed I
>>>>>>> think it is a better practice to use ones that are scaled with the
>>>>>>> jtreg timeout factors, etc. used to run the tests. Please instead use
>>>>>>> something like the adjustTimeout method of
>>>>>>>
>>>>>>> $JDK_FOREST_ROOT/test/lib/share/classes/jdk/test/lib/Utils
>>>>>>>
>>>>>>> As a general comment, I'd prefer we don't just up timeout values for
>>>>>>> tests. That can cause the whole test suite run to slow down, which is
>>>>>>> undesirable especially if the condition in question may actually be
>>>>>>> satisfied in many cases much faster than the timeout value.
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> -Joe
>>>>>>>
>>>>>>>
>>>>>>> On 7/7/2016 7:01 PM, Amy Lu wrote:
>>>>>>>
>>>>>>>> Please review this trivial fix for
>>>>>>>> test:java/lang/ThreadGroup/Stop.java
>>>>>>>>
>>>>>>>> Though this is a test for a deprecated API, failed with very very
>>>>>>>> low
>>>>>>>> frequency and hard to reproduce (I got no luck to reproduce it), I’d
>>>>>>>> like to patch it as suggested: extend the sleep in the main thread
>>>>>>>> from one second to five seconds. Also added 'volatile' to the
>>>>>>>> boolean
>>>>>>>> variable 'groupStopped'.
>>>>>>>>
>>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8132548
>>>>>>>> webrev: http://cr.openjdk.java.net/~amlu/8132548/webrev.00/
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Amy
>>>>>>>>
>>>>>>>>
>>>>>>>> --- old/test/java/lang/ThreadGroup/Stop.java 2016-07-04
>>>>>>>> 14:53:59.000000000 +0800
>>>>>>>> +++ new/test/java/lang/ThreadGroup/Stop.java 2016-07-04
>>>>>>>> 14:53:58.000000000 +0800
>>>>>>>> @@ -1,5 +1,5 @@
>>>>>>>> /*
>>>>>>>> - * Copyright (c) 1999, 2011, Oracle and/or its affiliates. All
>>>>>>>> rights reserved.
>>>>>>>> + * Copyright (c) 1999, 2016, Oracle and/or its affiliates. All
>>>>>>>> rights reserved.
>>>>>>>> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>>>>>>>> *
>>>>>>>> * This code is free software; you can redistribute it and/or
>>>>>>>> modify it
>>>>>>>> @@ -29,7 +29,7 @@
>>>>>>>> */
>>>>>>>>
>>>>>>>> public class Stop implements Runnable {
>>>>>>>> - private static boolean groupStopped = false ;
>>>>>>>> + private static volatile boolean groupStopped = false ;
>>>>>>>> private static final Object lock = new Object();
>>>>>>>>
>>>>>>>> private static final ThreadGroup group = new ThreadGroup("");
>>>>>>>> @@ -70,7 +70,7 @@
>>>>>>>> while (!groupStopped) {
>>>>>>>> lock.wait();
>>>>>>>> // Give the other thread a chance to stop
>>>>>>>> - Thread.sleep(1000);
>>>>>>>> + Thread.sleep(5000);
>>>>>>>> }
>>>>>>>> }
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>
>
