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








Reply via email to