Yes, but I just thought that for a test that testing a deprecated (since
1.2) API, and failed with very very low frequency (actually, only one
time during the years), we might not want to spend much effort on a big
change, like rewrite with CountDownLatch :-)
Thanks,
Amy
On 7/8/16 2:36 PM, Martin Buchholz wrote:
CountDownLatch is a better way of waiting for events, like for the two
threads to be started and for Thread.stop to have been called.
The test should ensure that ThreadDeath is indeed thrown. If the
threads in the group notify the main thread via a latch when they
catch ThreadDeath, then all the sleeps in this test can be removed.
On Thu, Jul 7, 2016 at 11:01 PM, Amy Lu <amy...@oracle.com
<mailto:amy...@oracle.com>> 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/
<http://cr.openjdk.java.net/%7Eamlu/8132548/webrev.01/>
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/
<http://cr.openjdk.java.net/%7Eamlu/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);
}
}
