JDK 9 RFR of JDK-8132548: java/lang/ThreadGroup/Stop.java fails with "RuntimeException: Failure"
David Holmes
david.holmes at oracle.com
Mon Jul 11 07:24:50 UTC 2016
My last email crossed with yours. Please see it.
David
On 11/07/2016 5:20 PM, Amy Lu wrote:
> 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 at oracle.com
>>> <mailto:joe.darcy at 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 at 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);
>>>>> }
>>>>> }
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
More information about the core-libs-dev
mailing list