8286830: ~HandshakeState should not touch oops

Reviewed-by: dholmes, rehn

Author: pchilano

src/hotspot/share/runtime/handshake.cpp

@@ -502,6 +502,16 @@ bool HandshakeState::has_async_exception_operation(bool ThreadDeath_only) {
   }
 }
 
+void HandshakeState::clean_async_exception_operation() {
+  while (has_async_exception_operation(/* ThreadDeath_only */ false)) {
+    MutexLocker ml(&_lock, Mutex::_no_safepoint_check_flag);
+    HandshakeOperation* op;
+    op = _queue.peek(async_exception_filter);
+    remove_op(op);
+    delete op;
+  }
+}
+
 bool HandshakeState::have_non_self_executable_operation() {
   assert(_handshakee != Thread::current(), "Must not be called by self");
   assert(_lock.owned_by_self(), "Lock must be held");

src/hotspot/share/runtime/handshake.hpp

@@ -133,6 +133,7 @@ class HandshakeState {
   bool has_operation() { return !_queue.is_empty(); }
   bool has_operation(bool allow_suspend, bool check_async_exception);
   bool has_async_exception_operation(bool ThreadDeath_only);
+  void clean_async_exception_operation();
 
   bool operation_pending(HandshakeOperation* op);
 

src/hotspot/share/runtime/thread.cpp

@@ -1365,6 +1365,7 @@ static bool is_daemon(oop threadObj) {
 // cleanup_failed_attach_current_thread as well.
 void JavaThread::exit(bool destroy_vm, ExitType exit_type) {
   assert(this == JavaThread::current(), "thread consistency check");
+  assert(!is_exiting(), "should not be exiting or terminated already");
 
   elapsedTimer _timer_exit_phase1;
   elapsedTimer _timer_exit_phase2;
@@ -1426,18 +1427,23 @@ void JavaThread::exit(bool destroy_vm, ExitType exit_type) {
     if (JvmtiExport::should_post_thread_life()) {
       JvmtiExport::post_thread_end(this);
     }
-
-    // The careful dance between thread suspension and exit is handled here.
-    // Since we are in thread_in_vm state and suspension is done with handshakes,
-    // we can just put in the exiting state and it will be correctly handled.
-    set_terminated(_thread_exiting);
-
-    ThreadService::current_thread_exiting(this, is_daemon(threadObj()));
   } else {
-    assert(!is_terminated() && !is_exiting(), "must not be exiting");
     // before_exit() has already posted JVMTI THREAD_END events
   }
 
+  // Cleanup any pending async exception now since we cannot access oops after
+  // BarrierSet::barrier_set()->on_thread_detach() has been executed.
+  if (has_async_exception_condition()) {
+    handshake_state()->clean_async_exception_operation();
+  }
+
+  // The careful dance between thread suspension and exit is handled here.
+  // Since we are in thread_in_vm state and suspension is done with handshakes,
+  // we can just put in the exiting state and it will be correctly handled.
+  // Also, no more async exceptions will be added to the queue after this point.
+  set_terminated(_thread_exiting);
+  ThreadService::current_thread_exiting(this, is_daemon(threadObj()));
+
   if (log_is_enabled(Debug, os, thread, timer)) {
     _timer_exit_phase1.stop();
     _timer_exit_phase2.start();
@@ -1529,7 +1535,8 @@ void JavaThread::exit(bool destroy_vm, ExitType exit_type) {
   }
 #endif // INCLUDE_JVMCI
 
-  // Remove from list of active threads list, and notify VM thread if we are the last non-daemon thread
+  // Remove from list of active threads list, and notify VM thread if we are the last non-daemon thread.
+  // We call BarrierSet::barrier_set()->on_thread_detach() here so no touching of oops after this point.
   Threads::remove(this, daemon);
 
   if (log_is_enabled(Debug, os, thread, timer)) {
@@ -1699,8 +1706,9 @@ void JavaThread::handle_async_exception(oop java_throwable) {
 }
 
 void JavaThread::install_async_exception(AsyncExceptionHandshake* aeh) {
-  // Do not throw asynchronous exceptions against the compiler thread.
-  if (!can_call_java()) {
+  // Do not throw asynchronous exceptions against the compiler thread
+  // or if the thread is already exiting.
+  if (!can_call_java() || is_exiting()) {
     delete aeh;
     return;
   }

test/hotspot/jtreg/runtime/Thread/StopAtExit.java

@@ -72,7 +72,30 @@ public static void main(String[] args) {
                 usage();
             }
         }
+        test(timeMax);
+
+        // Fire-up deamon that just creates new threads. This generates contention on
+        // Threads_lock while worker tries to exit, creating more places where target
+        // can be seen as handshake safe.
+        Thread threadCreator = new Thread() {
+            @Override
+            public void run() {
+                while (true) {
+                    Thread dummyThread = new Thread(() -> {});
+                    dummyThread.start();
+                    try {
+                        dummyThread.join();
+                    } catch(InterruptedException ie) {
+                    }
+                }
+            }
+        };
+        threadCreator.setDaemon(true);
+        threadCreator.start();
+        test(timeMax);
+    }
 
+    public static void test(int timeMax) {
         System.out.println("About to execute for " + timeMax + " seconds.");
 
         long count = 0;

test/hotspot/jtreg/runtime/Thread/SuspendAtExit.java

@@ -78,7 +78,30 @@ public static void main(String[] args) {
                     usage();
             }
         }
+        test(timeMax);
+
+        // Fire-up deamon that just creates new threads. This generates contention on
+        // Threads_lock while worker tries to exit, creating more places where target
+        // can be seen as handshake safe.
+        Thread threadCreator = new Thread() {
+            @Override
+            public void run() {
+                while (true) {
+                    Thread dummyThread = new Thread(() -> {});
+                    dummyThread.start();
+                    try {
+                        dummyThread.join();
+                    } catch(InterruptedException ie) {
+                    }
+                }
+            }
+        };
+        threadCreator.setDaemon(true);
+        threadCreator.start();
+        test(timeMax);
+    }
 
+    public static void test(int timeMax) {
         System.out.println("About to execute for " + timeMax + " seconds.");
 
         long count = 0;