On Tue, 4 Apr 2023 09:22:29 GMT, Aleksey Shipilev <sh...@openjdk.org> wrote:
>> David Holmes has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Ensure HB relationship exists; additional explanatory comments
>
> src/java.base/share/classes/java/lang/Thread.java line 1850:
>
>> 1848: * happens-before isAlive() returning false.
>> 1849: */
>> 1850: synchronized boolean alive() {
>
> Ah. Yes, it needs to be synchronized on the same lock that VM uses, which is
> `this`.
>
> But there is also a secondary issue fixed by this synchronization: without
> it, a simple `while (t.isAlive()) {}` loop would execute indefinitely, since
> `eetop` is going to be hoisted. This can be reproduced with the test below.
>
> Consider adding it as `test/jdk/java/lang/Thread/IsAlive.java`:
>
>
> /*
> * Copyright 2023, Amazon.com Inc. 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
> * under the terms of the GNU General Public License version 2 only, as
> * published by the Free Software Foundation.
> *
> * This code is distributed in the hope that it will be useful, but WITHOUT
> * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> * version 2 for more details (a copy is included in the LICENSE file that
> * accompanied this code).
> *
> * You should have received a copy of the GNU General Public License version
> * 2 along with this work; if not, write to the Free Software Foundation,
> * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
> *
> * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
> * or visit www.oracle.com if you need additional information or have any
> * questions.
> */
>
> /*
> * @test
> * @bug 8305425
> * @summary Check Thread.isAlive
> * @run main/othervm/timeout=10 IsAlive
> */
>
> public class IsAlive {
>
> private static void spin() {
> try {
> while (!Thread.currentThread().isInterrupted()) {
> Thread.sleep(10);
> }
> } catch (InterruptedException ie) {
> // Do nothing, just exit
> }
> }
>
> static volatile boolean checkerReady;
>
> private static void check(Thread t) {
> while (!t.isAlive()) {
> // Burn hard, without any sleeps.
> // Check that we discover the thread is alive eventually.
> }
>
> checkerReady = true;
>
> while (t.isAlive()) {
> // Burn hard, without any sleeps.
> // Check that we discover the thread is not alive eventually.
> }
> }
>
> private static void assertAlive(Thread t) {
> if (!t.isAlive()) {
> throw new IllegalStateException("Thread " + t + " is not alive,
> but it should be");
> }
> }
>
> private static void assertNotAlive(Thread t) {
> if (t.isAlive()) {
> throw new IllegalStateException("Thread " + t + " is alive, but
> it should not be");
> }
> }
>
> public static void main(String args[]) throws Exception {
> Thread spinner = new Thread(IsAlive::spin);
> spinner.setName("Spinner");
> spinner.setDaemon(true);
>
> Thread checker = new Thread(() -> check(spinner));
> checker.setName("Checker");
> checker.setDaemon(true);
>
> assertNotAlive(spinner);
> assertNotAlive(checker);
>
> System.out.println("Starting spinner");
> spinner.start();
> assertAlive(spinner);
>
> System.out.println("Starting checker");
> checker.start();
> assertAlive(checker);
>
> System.out.println("Waiting for checker to catch up");
> while (!checkerReady) {
> Thread.sleep(100);
> }
>
> System.out.println("Interrupting and joining spinner");
> spinner.interrupt();
> spinner.join();
> assertNotAlive(spinner);
>
> System.out.println("Joining checker");
> checker.join();
> assertNotAlive(checker);
>
> System.out.println("Complete");
> }
> }
Yes good catch @shipilev ! The fact the field was set by the VM dealt with a
couple of issues that direct setting has to deal with explicitly. I will
incorporate your new test. Thanks.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13287#discussion_r1157826819
