On 7/11/16 4: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/
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.
The time LONG_DELAY_MS used in join(long millis) is an adjusted timeout:
private static final long LONG_DELAY_MS = Utils.adjustTimeout(1000L);
Typo.
private static final long LONG_DELAY_MS = Utils.adjustTimeout(5000L);
Thanks,
Amy
...
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);
}
}
