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/ 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); } }

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.lu@oracle.com <mailto:amy.lu@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); } }

jdk/test/java/util/concurrent/tck has thousands of test methods. It used to take minutes to run them all, but now only takes 10 seconds, mostly due to replacements of sleeps with faster and more robust alternatives, often CountDownLatch. On Fri, Jul 8, 2016 at 10:17 AM, joe darcy <joe.darcy@oracle.com> wrote:

The most surefire way to make sure the test doesn't fail anymore is to hg rm the test; if and unless the code is actually removed, that would not be the most appropriate approach though ;-)

While it might be overkill for this particular test, I think it would be preferable to start replacing our various sleep calls in tests with count down latches or other structures as appropriate. Converting this test could help provide a good example of the process.

(As alluded to earlier, the test suite as a whole could be made to run somewhat faster. Tests which wait for seconds when the entire test could commonly run in milliseconds in many cases are proportionately a good candidate to speed up. The absolute wait time are also problematic on the other extreme, running under -Xint on a heavily loaded test system, "should never take this long" numbers often aren't enough.)

Thanks,

-Joe

On 7/7/2016 11:46 PM, Amy Lu wrote:

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.lu@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/

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); } }

Please review the updated webrev: http://cr.openjdk.java.net/~amlu/8132548/webrev.02/ Thanks, Amy On 7/11/16 9:20 AM, Amy Lu wrote:

Thank you for all the valuable comments.

I'm updating the webrev...

Thanks, Amy

On 7/9/16 1:34 AM, Martin Buchholz wrote:

...

jdk/test/java/util/concurrent/tck has thousands of test methods. It used to take minutes to run them all, but now only takes 10 seconds, mostly due to replacements of sleeps with faster and more robust alternatives, often CountDownLatch.

On Fri, Jul 8, 2016 at 10:17 AM, joe darcy <joe.darcy@oracle.com <mailto:joe.darcy@oracle.com>> wrote:

The most surefire way to make sure the test doesn't fail anymore is to hg rm the test; if and unless the code is actually removed, that would not be the most appropriate approach though ;-)

While it might be overkill for this particular test, I think it would be preferable to start replacing our various sleep calls in tests with count down latches or other structures as appropriate. Converting this test could help provide a good example of the process.

(As alluded to earlier, the test suite as a whole could be made to run somewhat faster. Tests which wait for seconds when the entire test could commonly run in milliseconds in many cases are proportionately a good candidate to speed up. The absolute wait time are also problematic on the other extreme, running under -Xint on a heavily loaded test system, "should never take this long" numbers often aren't enough.)

Thanks,

-Joe

On 7/7/2016 11:46 PM, Amy Lu wrote:

...

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.lu@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); } }

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. 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); } }

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); ... 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); } }

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); } }

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.lu@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); >> } >> } >> >> >> >> >

On 12/07/2016 4:17 PM, Martin Buchholz wrote:

Amy, sorry you have so many picky reviewers!

Difference between patching a failing test and completely redesigning it from scratch. The latter was not warranted in my opinion.

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<>();

Simple volatile Throwable fields would suffice - AtomicReferences are overkill.

private static final Thread second = new Thread(group, () -> { ready.countDown(); try {

You have to place the countDown() inside the try block else the ThreadDeath may escape.

do {} while (true);

Busy-loops are "bad".

} 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. } }

I agree this meets the "spec" of checking "did group.stop() cause all group members to encounter a ThreadDeath exception". But in relation to fixing the original timing problem, well a simple untimed-join() fixed that. David -----

On Mon, Jul 11, 2016 at 10:28 PM, Amy Lu <amy.lu@oracle.com <mailto:amy.lu@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); } }

On 7/12/16 2:17 PM, Martin Buchholz wrote:

Amy, sorry you have so many picky reviewers! My lucky ;-)

Additional checking for ThreadDeath may good, but with the thinking that both group.stop() and thread.stop() are deprecated (since 1.2), I don't want to introduce new testcase, but focus on "fixing". The the unnecessary statics removed, thanks for point that out. Thanks, Amy

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.lu@oracle.com <mailto:amy.lu@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/ <http://cr.openjdk.java.net/%7Eamlu/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/ <http://cr.openjdk.java.net/%7Eamlu/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/ <http://cr.openjdk.java.net/%7Eamlu/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/ <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); } }

Updated on the busy-loops, also removed the unnecessary statics, here comes the updated webrev: http://cr.openjdk.java.net/~amlu/8132548/webrev.04/ Thanks, Amy On 7/12/16 2:33 PM, David Holmes wrote:

Hi Amy,

On 12/07/2016 3:28 PM, Amy Lu wrote:

...

Please help to review the updated webrev, getting rid of ThreadDeath and using join():

http://cr.openjdk.java.net/~amlu/8132548/webrev.03/

I still think a simple change to the original code suffices, but given you have gone this far ...

Busy-loops are "bad" and unnecessary:

40 while (true) { // keep it alive 41 }

This can be changed to a long blocking operation eg:

while (true) { try { Thread.sleep(60000); } catch (InterruptedException shouldNotHappen) { } }

Thanks, David

...

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); >>> } >>> } >>> >>> >>> >> >