Re: JDK 9 RFR of JDK-8132548: java/lang/ThreadGroup/Stop.java fails with "RuntimeException: Failure"

[Amy Lu]

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