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
