8304147: JVM crash during shutdown when dumping dynamic archive

Reviewed-by: ccheung, matsaave, coleenp

Author: David Holmes

src/hotspot/share/cds/dynamicArchive.cpp

@@ -377,18 +377,36 @@ void DynamicArchive::check_for_dynamic_dump() {
   }
 }
 
-void DynamicArchive::prepare_for_dump_at_exit() {
-  EXCEPTION_MARK;
-  ResourceMark rm(THREAD);
+void DynamicArchive::dump_at_exit(JavaThread* current, const char* archive_name) {
+  ExceptionMark em(current);
+  ResourceMark rm(current);
+  HandleMark hm(current);
+
+  if (!DynamicDumpSharedSpaces || archive_name == nullptr) {
+    return;
+  }
+
+  log_info(cds, dynamic)("Preparing for dynamic dump at exit in thread %s", current->name());
+
+  JavaThread* THREAD = current; // For TRAPS processing related to link_shared_classes
   MetaspaceShared::link_shared_classes(false/*not from jcmd*/, THREAD);
-  if (HAS_PENDING_EXCEPTION) {
-    log_error(cds)("Dynamic dump has failed");
-    log_error(cds)("%s: %s", PENDING_EXCEPTION->klass()->external_name(),
-                   java_lang_String::as_utf8_string(java_lang_Throwable::message(PENDING_EXCEPTION)));
-    // We cannot continue to dump the archive anymore.
-    DynamicDumpSharedSpaces = false;
-    CLEAR_PENDING_EXCEPTION;
+  if (!HAS_PENDING_EXCEPTION) {
+    // copy shared path table to saved.
+    FileMapInfo::clone_shared_path_table(current);
+    if (!HAS_PENDING_EXCEPTION) {
+      VM_PopulateDynamicDumpSharedSpace op(archive_name);
+      VMThread::execute(&op);
+      return;
+    }
   }
+
+  // One of the prepatory steps failed
+  oop ex = current->pending_exception();
+  log_error(cds)("Dynamic dump has failed");
+  log_error(cds)("%s: %s", ex->klass()->external_name(),
+                 java_lang_String::as_utf8_string(java_lang_Throwable::message(ex)));
+  CLEAR_PENDING_EXCEPTION;
+  DynamicDumpSharedSpaces = false;  // Just for good measure
 }
 
 // This is called by "jcmd VM.cds dynamic_dump"
@@ -397,21 +415,12 @@ void DynamicArchive::dump_for_jcmd(const char* archive_name, TRAPS) {
   assert(ArchiveClassesAtExit == nullptr, "already checked in arguments.cpp");
   assert(DynamicDumpSharedSpaces, "already checked by check_for_dynamic_dump() during VM startup");
   MetaspaceShared::link_shared_classes(true/*from jcmd*/, CHECK);
-  dump(archive_name, THREAD);
-}
-
-void DynamicArchive::dump(const char* archive_name, TRAPS) {
   // copy shared path table to saved.
   FileMapInfo::clone_shared_path_table(CHECK);
-
   VM_PopulateDynamicDumpSharedSpace op(archive_name);
   VMThread::execute(&op);
 }
 
-bool DynamicArchive::should_dump_at_vm_exit() {
-  return DynamicDumpSharedSpaces && (ArchiveClassesAtExit != nullptr);
-}
-
 bool DynamicArchive::validate(FileMapInfo* dynamic_info) {
   assert(!dynamic_info->is_static(), "must be");
   // Check if the recorded base archive matches with the current one

src/hotspot/share/cds/dynamicArchive.hpp

@@ -59,10 +59,8 @@ class DynamicArchiveHeader : public FileMapHeader {
 class DynamicArchive : AllStatic {
 public:
   static void check_for_dynamic_dump();
-  static bool should_dump_at_vm_exit();
-  static void prepare_for_dump_at_exit();
   static void dump_for_jcmd(const char* archive_name, TRAPS);
-  static void dump(const char* archive_name, TRAPS);
+  static void dump_at_exit(JavaThread* current, const char* archive_name);
   static bool is_mapped() { return FileMapInfo::dynamic_info() != nullptr; }
   static bool validate(FileMapInfo* dynamic_info);
 };

src/hotspot/share/prims/jvm.cpp

@@ -426,12 +426,6 @@ JVM_END
 extern volatile jint vm_created;
 
 JVM_ENTRY_NO_ENV(void, JVM_BeforeHalt())
-#if INCLUDE_CDS
-  // Link all classes for dynamic CDS dumping before vm exit.
-  if (DynamicArchive::should_dump_at_vm_exit()) {
-    DynamicArchive::prepare_for_dump_at_exit();
-  }
-#endif
   EventShutdown event;
   if (event.should_commit()) {
     event.set_reason("Shutdown requested from Java");

src/hotspot/share/runtime/java.cpp

@@ -439,6 +439,14 @@ void before_exit(JavaThread* thread, bool halt) {
   }
 #endif
 
+#if INCLUDE_CDS
+  // Dynamic CDS dumping must happen whilst we can still reliably
+  // run Java code.
+  DynamicArchive::dump_at_exit(thread, ArchiveClassesAtExit);
+  assert(!thread->has_pending_exception(), "must be");
+#endif
+
+
   // Actual shutdown logic begins here.
 
 #if INCLUDE_JVMCI
@@ -514,21 +522,6 @@ void before_exit(JavaThread* thread, bool halt) {
   // Note: we don't wait until it actually dies.
   os::terminate_signal_thread();
 
-#if INCLUDE_CDS
-  if (DynamicArchive::should_dump_at_vm_exit()) {
-    assert(ArchiveClassesAtExit != nullptr, "Must be already set");
-    ExceptionMark em(thread);
-    DynamicArchive::dump(ArchiveClassesAtExit, thread);
-    if (thread->has_pending_exception()) {
-      ResourceMark rm(thread);
-      oop pending_exception = thread->pending_exception();
-      log_error(cds)("ArchiveClassesAtExit has failed %s: %s", pending_exception->klass()->external_name(),
-                     java_lang_String::as_utf8_string(java_lang_Throwable::message(pending_exception)));
-      thread->clear_pending_exception();
-    }
-  }
-#endif
-
   print_statistics();
   Universe::heap()->print_tracing_info();
 

src/hotspot/share/runtime/javaThread.cpp

@@ -2023,22 +2023,11 @@ bool JavaThread::sleep(jlong millis) {
 void JavaThread::invoke_shutdown_hooks() {
   HandleMark hm(this);
 
-  // We could get here with a pending exception, if so clear it now or
-  // it will cause MetaspaceShared::link_shared_classes to
-  // fail for dynamic dump.
+  // We could get here with a pending exception, if so clear it now.
   if (this->has_pending_exception()) {
     this->clear_pending_exception();
   }
 
-#if INCLUDE_CDS
-  // Link all classes for dynamic CDS dumping before vm exit.
-  // Same operation is being done in JVM_BeforeHalt for handling the
-  // case where the application calls System.exit().
-  if (DynamicArchive::should_dump_at_vm_exit()) {
-    DynamicArchive::prepare_for_dump_at_exit();
-  }
-#endif
-
   EXCEPTION_MARK;
   Klass* shutdown_klass =
     SystemDictionary::resolve_or_null(vmSymbols::java_lang_Shutdown(),

test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/ExitRaceTest.java

@@ -0,0 +1,59 @@
+/*
+ * Copyright (c) 2023, 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 8304147
+ * @summary Test the exit race for dynamic dumping at exit
+ * @requires vm.cds
+ * @library /test/lib /test/hotspot/jtreg/runtime/cds/appcds /test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/test-classes
+ * @build ExitRace jdk.test.whitebox.WhiteBox
+ * @run driver jdk.test.lib.helpers.ClassFileInstaller -jar exitRace.jar ExitRace ExitRace$1 ExitRace$1$1
+ * @run driver jdk.test.lib.helpers.ClassFileInstaller jdk.test.whitebox.WhiteBox
+ * @run main/othervm -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI -Xbootclasspath/a:. ExitRaceTest
+ */
+
+import jdk.test.lib.helpers.ClassFileInstaller;
+
+public class ExitRaceTest extends DynamicArchiveTestBase {
+
+    public static void main(String[] args) throws Exception {
+        runTest(ExitRaceTest::test);
+    }
+
+    static void test() throws Exception {
+        String topArchiveName = getNewArchiveName();
+        String appJar = ClassFileInstaller.getJarPath("exitRace.jar");
+        String mainClass = "ExitRace";
+
+        dump(topArchiveName,
+             "-Xlog:cds+dynamic=debug",
+             "-cp", appJar, mainClass)
+            .assertNormalExit(output -> {
+                                output.shouldHaveExitValue(0)
+                                      .shouldContain("Preparing for dynamic dump")
+                                      .reportDiagnosticSummary();
+                              });
+    }
+}

test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/test-classes/ExitRace.java

@@ -0,0 +1,71 @@
+/*
+ * Copyright (c) 2023, 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.
+ *
+ */
+
+// Try to trigger concurrent dynamic-dump-at-exit processing by creating a race
+// between normal VM termination by the last non-daemon thread exiting, and a
+// call to Runtime.halt().
+
+public class ExitRace {
+
+    static volatile int terminationPhase = 0;
+
+    public static void main(String [] args) {
+
+        // Need to spawn a new non-daemon thread so the main thread will
+        // have time to become the DestroyJavaVM thread.
+        Thread runner = new Thread("Runner") {
+                public void run() {
+                    // This thread will be the one to trigger normal VM termination
+                    // when it exits. We first create a daemon thread to call
+                    // Runtime.halt() and then wait for it to tell us to exit.
+
+                    Thread daemon = new Thread("Daemon") {
+                            public void run() {
+                                // Let main thread go
+                                terminationPhase = 1;
+                                // Spin until main thread is active again
+                                while (terminationPhase == 1)
+                                    ;
+                                Runtime.getRuntime().halt(0); // Normal exit code
+                            }
+                        };
+                    daemon.setDaemon(true);
+                    daemon.start();
+
+                    // Wait until daemon is ready
+                    while (terminationPhase == 0) {
+                        try {
+                            Thread.sleep(10);
+                        } catch (InterruptedException cantHappen) {
+                        }
+                    }
+
+                    // Release daemon thread
+                    terminationPhase++;
+                    // Normal exit
+                }
+            };
+        runner.start();
+    }
+}