On 7/12/16 2:17 PM, Martin Buchholz wrote:
Amy, sorry you have so many picky reviewers!
My lucky ;-)

Additional checking for ThreadDeath may good, but with the thinking that both group.stop() and thread.stop() are deprecated (since 1.2), I don't want to introduce new testcase, but focus on "fixing".

The the unnecessary statics removed, thanks for point that out.

Thanks,
Amy

Here's how I might write it:

import java.util.concurrent.CountDownLatch;
import java.util.concurrent.atomic.AtomicReference;

public class Stop {
    private static final CountDownLatch ready = new CountDownLatch(1);
    private static final ThreadGroup group = new ThreadGroup("");
    private static final AtomicReference<Throwable> firstThrowable
        = new AtomicReference<>();
    private static final AtomicReference<Throwable> secondThrowable
        = new AtomicReference<>();

    private static final Thread second = new Thread(group, () -> {
        ready.countDown();
        try {
            do {} while (true);
        } catch (Throwable ex) {
            secondThrowable.set(ex);
        }
    });

    private static final Thread first = new Thread(group, () -> {
        // Wait until "second" is started
        try {
            ready.await();
            group.stop();
        } catch (Throwable ex) {
            firstThrowable.set(ex);
        }
    });

    public static void main(String[] args) throws Exception {
        // Launch two threads as part of the same thread group
        first.start();
        second.start();

        // Both threads should terminate when the first thread
        // terminates the thread group.
        second.join();
        first.join();
        if (! (firstThrowable.get() instanceof ThreadDeath) ||
            ! (secondThrowable.get() instanceof ThreadDeath))
            throw new AssertionError("should have thrown ThreadDeath");
        // Test passed - if never get here the test times out and fails.
    }
}


On Mon, Jul 11, 2016 at 10:28 PM, Amy Lu <amy...@oracle.com <mailto:amy...@oracle.com>> wrote:

    Please help to review the updated webrev, getting rid of
    ThreadDeath and using join():

    http://cr.openjdk.java.net/~amlu/8132548/webrev.03/
    <http://cr.openjdk.java.net/%7Eamlu/8132548/webrev.03/>

    Thanks,
    Amy


    On 7/11/16 6:55 PM, David Holmes wrote:



        On 11/07/2016 6:14 PM, Amy Lu wrote:

            Thank you David, though the email crossed-by, I hope all
            the concerns
            have been resolved in the updated webrev:
            http://cr.openjdk.java.net/~amlu/8132548/webrev.02/
            <http://cr.openjdk.java.net/%7Eamlu/8132548/webrev.02/>


        Sorry but no, the changes are neither necessary nor
        sufficient. With the new code the ThreadDeath can hit anytime
        after countDown(), so it can hit before you enter the try block.

            I had rewrote the test with CountDownLatch and join(long
            millis). Also
            unlike the old version, test thread 'first' and 'second'
            do different
            things, and 'second' thread will keep alive for waiting to
            be killed by
            group.stop() from 'frist' thread in the same thread group.
            I think this
            could avoid the second call to second.stop()
            (group.stop()) issue.


        Simply using join() with no timeout and relying on the test
        framework to kill the test allows you to do away with the
        second stop().

        David
        -----

            The time LONG_DELAY_MS used in join(long millis) is an
            adjusted timeout:

            private static final long LONG_DELAY_MS =
            Utils.adjustTimeout(1000L);
            ...

                    second.join(LONG_DELAY_MS);
                    boolean failed = second.isAlive();

            This gives second thread a more "reasonable" time to die,
            with taking
            account of the test harness timeout.
            This almost same as second.join()// test pass or timeout,
            but with
            additional chance of killing all threads in a bad case.

            Thanks,
            Amy

            On 7/11/16 3:25 PM, David Holmes wrote:

                Simplification ...

                On 11/07/2016 5:12 PM, David Holmes wrote:

                    Hi Amy,

                    Thanks for tackling this.

                    On 8/07/2016 4:01 PM, Amy Lu 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/>


                    The retry serves no purpose here. groupStopped is
                    being set to true and
                    the waiting thread has been notified, so the loop
                    will terminate after
                    the first sleep. The failure happens when the main
                    thread checks the
                    isAlive() status of the second thread before the
                    ThreadGroup.stop has
                    actually had a chance to stop and terminate it -
                    such that isAlive is
                    now false. That is why I suggested waiting a bit
                    longer by extending the
                    sleep.

                    I agree that making the test last at least 5
                    seconds is not ideal, but I
                    didn't think it would be an issue in the big
                    picture of testing.

                    Ideally explicit "synchronization" is better than
                    sleeps but that would
                    again be missing the point with this test. We
                    expect the thread to
                    terminate, if it hasn't terminated in a
                    "reasonable" time we consider
                    the stop() to have failed and the test to fail. To
                    that end we could
                    remove the sleep altogether and change:

                    boolean failed = second.isAlive();

                    to

                    try {
                      second.join(1000);
                    } catch (InterruptedException shouldNotHappen) {}
                    boolean failed = second.isAlive();

                    Now we use explicit event synchronization - great!
                    But the test has the
                    same failure issue now as it did with the sleep.
                    Putting in a
                    CountDownLatch would have exactly the same
                    problem: we expect the second
                    thread to signal the latch as it terminates, but
                    if that doesn't happen
                    within a "reasonable" amount of time, then we deem
                    the stop() to have
                    failed and the test to have failed.

                    Also note that the more code we add the more
                    likely the second call to
                    second.stop() triggers an async exception in code
                    that can't handle it
                    and results in an even worse failure mode!

                    The only thing I can suggest is to get rid of the
                    explicit sleep (or
                    join, or latch.await() etc) and simply recode as
                    an infinite loop and
                    rely on the test harness to tear things down when
                    the overall test
                    timeout kicks in. That way the test either passes
                    or is timed-out, there
                    is no explicit failure - but a busy loop is also
                    bad so you would want a
                    small sleep in there anyway:

                    while (second.isAlive()) {
                      Thread.sleep(10);
                    }
                    // Test passed - if we never get here the test
                    times out and
                    // we implicitly fail.


                Of course that was silly all you need is:

                second.join();
                // Test passed - if we never get here the test times
                out and
                // we implicitly fail.

                David


                    Thanks,
                    David

                        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