On Mon, 3 Apr 2023 22:41:44 GMT, David Holmes <dhol...@openjdk.org> wrote:
>> We have the strange situation where calling `t.isAlive()` on a >> `java.lang.Thread` `t`, will call into the VM (via `alive()` then >> `isAlive0()`) where the VM then examines the `eetop` field of `t` to extract >> its `JavaThread` pointer and compare it to null. We can simply read `eetop` >> directly in `Thread.alive()`: >> >> boolean alive() { >> return eetop != 0; >> } >> >> I also updated a comment in relation to `eetop`. >> >> Testing: tiers 1-3 >> >> Thanks > > 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 231: > 229: /* Reserved for exclusive use by the JVM. Cannot be moved to the > FieldHolder > 230: as it needs to be set by the VM before executing the constructor > that > 231: will create the FieldHolder. The historically named `eetop`holds > the address Suggestion: will create the FieldHolder. The historically named `eetop` holds the address 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"); } } ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/13287#discussion_r1156901698 PR Review Comment: https://git.openjdk.org/jdk/pull/13287#discussion_r1156973465