JDK 9 RFR of JDK-8132548: java/lang/ThreadGroup/Stop.java fails with "RuntimeException: Failure"
Amy Lu
amy.lu at oracle.com
Mon Jul 11 07:20:13 UTC 2016
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