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