8290732: JNI DestroyJavaVM can start shutdown when one non-daemon thread remains

Reviewed-by: stuefe, rehn

Author: David Holmes

make/test/JtregNativeHotspot.gmk

@@ -1043,10 +1043,14 @@ void Threads::destroy_vm() {
 #ifdef ASSERT
   _vm_complete = false;
 #endif
-  // Wait until we are the last non-daemon thread to execute
+  // Wait until we are the last non-daemon thread to execute, or
+  // if we are a daemon then wait until the last non-daemon thread has
+  // executed.
+  bool daemon = java_lang_Thread::is_daemon(thread->threadObj());
+  int expected = daemon ? 0 : 1;
   {
     MonitorLocker nu(Threads_lock);
-    while (Threads::number_of_non_daemon_threads() > 1)
+    while (Threads::number_of_non_daemon_threads() > expected)
       // This wait should make safepoint checks, wait without a timeout.
       nu.wait(0);
   }
@@ -1223,9 +1227,11 @@ void Threads::remove(JavaThread* p, bool is_daemon) {
     if (!is_daemon) {
       _number_of_non_daemon_threads--;
 
-      // Only one thread left, do a notify on the Threads_lock so a thread waiting
-      // on destroy_vm will wake up.
-      if (number_of_non_daemon_threads() == 1) {
+      // If this is the last non-daemon thread then we need to do
+      // a notify on the Threads_lock so a thread waiting
+      // on destroy_vm will wake up. But that thread could be a daemon
+      // or non-daemon, so we notify for both the 0 and 1 case.
+      if (number_of_non_daemon_threads() <= 1) {
         ml.notify_all();
       }
     }

src/hotspot/share/runtime/threads.cpp

@@ -0,0 +1,56 @@
+/*
+ * Copyright (c) 2022, 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
+ * 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.
+ *
+ */
+
+/**
+ * Simple application that checks that shutdown does not commence until
+ * after the last non-daemon thread has terminated. Reporting failure is
+ * tricky because we can't uses exceptions (as they are ignored from Shutdown
+ * hooks) and we can't call System.exit. So we rely on System.out being checked
+ * in the TestDaemonDestroy application.
+ */
+public class Main {
+
+    static volatile Thread t1;
+
+    public static void main() {
+        t1 = new Thread(() -> {
+                System.out.println("T1 started");
+                try {
+                    Thread.sleep(5000);
+                } catch (InterruptedException ignore) { }
+                System.out.println("T1 finished");
+            }, "T1");
+
+        t1.setDaemon(false);
+
+        Thread hook = new Thread(() -> {
+                System.out.println("HOOK started");
+                if (t1.isAlive()) {
+                    System.out.println("Error: T1 isAlive");
+                }
+            }, "HOOK");
+        Runtime.getRuntime().addShutdownHook(hook);
+        t1.start();
+    }
+}

test/hotspot/jtreg/runtime/jni/daemonDestroy/Main.java

@@ -0,0 +1,81 @@
+/*
+ * Copyright (c) 2022, 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
+ * 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 8290732
+ * @comment Test uses custom launcher that attempts to destroy the VM on both
+ *          a daemon and non-daemon thread. The result should be the same in
+ *          both cases.
+ * @requires vm.flagless
+ * @library /test/lib
+ * @build Main
+ * @run main/native TestDaemonDestroy
+ * @run main/native TestDaemonDestroy daemon
+ */
+
+// Logic copied from SigTestDriver
+
+import jdk.test.lib.Platform;
+import jdk.test.lib.Utils;
+import jdk.test.lib.process.OutputAnalyzer;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Paths;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.List;
+
+public class TestDaemonDestroy {
+
+    public static void main(String[] args) throws IOException {
+        Path launcher = Paths.get(Utils.TEST_NATIVE_PATH)
+            .resolve("daemonDestroy" + (Platform.isWindows() ? ".exe" : ""))
+            .toAbsolutePath();
+
+        System.out.println("Launcher = " + launcher +
+                           (Files.exists(launcher) ? " (exists)" : " (missing)"));
+
+        List<String> cmd = new ArrayList<>();
+        cmd.add(launcher.toString());
+        cmd.add("-Djava.class.path=" + Utils.TEST_CLASS_PATH);
+        if (args.length > 0) {
+            cmd.add("daemon");
+        }
+        ProcessBuilder pb = new ProcessBuilder(cmd);
+
+        // Need to add libjvm location to LD_LIBRARY_PATH
+        String envVar = Platform.sharedLibraryPathVariableName();
+        pb.environment().merge(envVar, Platform.jvmLibDir().toString(),
+                               (x, y) -> y + File.pathSeparator + x);
+
+        OutputAnalyzer oa = new OutputAnalyzer(pb.start());
+        oa.shouldHaveExitValue(0);
+        oa.shouldNotContain("Error: T1 isAlive");
+        oa.shouldContain("T1 finished");
+        oa.reportDiagnosticSummary();
+    }
+}

test/hotspot/jtreg/runtime/jni/daemonDestroy/TestDaemonDestroy.java

@@ -0,0 +1,105 @@
+/*
+ * Copyright (c) 2022, 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
+ * 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.
+ */
+
+#include <jni.h>
+#include <stdlib.h>
+
+static JavaVMOption options[] = {
+  { "-Djava.class.path=.", NULL }, // gets overwritten with real value
+};
+
+static JavaVMInitArgs vm_args = {
+  JNI_VERSION_19,
+  sizeof(options) / sizeof(JavaVMOption),
+  options,
+  JNI_FALSE
+};
+
+int main(int argc, char *argv[]) {
+  JavaVM *jvm;
+  JNIEnv *env;
+
+  if (argc < 2) {
+    fprintf(stderr, "Usage: main <classpath property> [daemon]\n");
+    exit(1);
+  }
+
+  char* cp = argv[1];
+
+  printf("Test using classpath: %s\n", cp);
+
+  options[0].optionString = cp;
+
+  jint res = JNI_CreateJavaVM(&jvm, (void**)&env, &vm_args);
+  if (res != JNI_OK) {
+    fprintf(stderr, "Test Error: JNI_CreateJavaVM failed: %d\n", res);
+    exit(1);
+  }
+
+  jclass cls = (*env)->FindClass(env, "Main");
+  if (cls == NULL) {
+    fprintf(stderr, "Test Error. Can't load class Main\n");
+    (*env)->ExceptionDescribe(env);
+    exit(1);
+  }
+
+  jmethodID mid = (*env)->GetStaticMethodID(env, cls, "main", "()V");
+  if (mid == NULL) {
+    fprintf(stderr, "Test Error. Can't find method main\n");
+    (*env)->ExceptionDescribe(env);
+    exit(1);
+  }
+
+  (*env)->CallStaticVoidMethod(env, cls, mid);
+
+  res = (*jvm)->DetachCurrentThread(jvm);
+  if (res != JNI_OK) {
+    fprintf(stderr, "Test Error: DetachCurrentThread failed: %d\n", res);
+    exit(1);
+  }
+
+  // Any additional arg implies to use a daemon thread.
+  if (argc > 2) {
+    res = (*jvm)->AttachCurrentThreadAsDaemon(jvm, (void **)&env, NULL);
+    if (res != JNI_OK) {
+      fprintf(stderr, "Test Error: AttachCurrentThreadAsDaemon failed: %d\n", res);
+      exit(1);
+    }
+    puts("Test: attached as daemon");
+  } else {
+    res = (*jvm)->AttachCurrentThread(jvm, (void **)&env, NULL);
+    if (res != JNI_OK) {
+      fprintf(stderr, "Test Error: AttachCurrentThread failed: %d\n", res);
+      exit(1);
+    }
+    puts("Test: attached as non-daemon");
+  }
+
+  puts("Test: calling DestroyJavaVM");
+  res = (*jvm)->DestroyJavaVM(jvm);
+  if (res != JNI_OK) {
+    fprintf(stderr, "Test Error: DestroyJavaVM failed: %d\n", res);
+    exit(1);
+  }
+  puts("Test: DestroyJavaVM returned");
+}