RFR: 8305425: Thread.isAlive0 doesn't need to call into the VM [v3]

[David Holmes]

On Tue, 4 Apr 2023 09:22:29 GMT, Aleksey Shipilev <shade at 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