From 4a79270c0195e79cfc59f774c1ac58c56321ea8a Mon Sep 17 00:00:00 2001 From: "Daniel D. Daugherty" Date: Mon, 2 May 2022 15:23:03 +0000 Subject: [PATCH] 8284632: runtime/Thread/StopAtExit.java possibly leaking memory again Reviewed-by: pchilanomate, rehn --- src/hotspot/share/runtime/handshake.cpp | 56 +++++++++++-------- src/hotspot/share/runtime/handshake.hpp | 1 + src/hotspot/share/runtime/thread.cpp | 4 ++ .../jtreg/runtime/Thread/StopAtExit.java | 18 +++++- 4 files changed, 53 insertions(+), 26 deletions(-) diff --git a/src/hotspot/share/runtime/handshake.cpp b/src/hotspot/share/runtime/handshake.cpp index c2f91016b4b..e9f75896c84 100644 --- a/src/hotspot/share/runtime/handshake.cpp +++ b/src/hotspot/share/runtime/handshake.cpp @@ -424,29 +424,6 @@ void Handshake::execute(AsyncHandshakeClosure* hs_cl, JavaThread* target) { target->handshake_state()->add_operation(op); } -HandshakeState::HandshakeState(JavaThread* target) : - _handshakee(target), - _queue(), - _lock(Monitor::nosafepoint, "HandshakeState_lock"), - _active_handshaker(), - _async_exceptions_blocked(false), - _suspended(false), - _async_suspend_handshake(false) -{ -} - -void HandshakeState::add_operation(HandshakeOperation* op) { - // Adds are done lock free and so is arming. - _queue.push(op); - SafepointMechanism::arm_local_poll_release(_handshakee); -} - -bool HandshakeState::operation_pending(HandshakeOperation* op) { - MutexLocker ml(&_lock, Mutex::_no_safepoint_check_flag); - MatchOp mo(op); - return _queue.contains(mo); -} - // Filters static bool non_self_executable_filter(HandshakeOperation* op) { return !op->is_async(); @@ -463,6 +440,39 @@ static bool is_ThreadDeath_filter(HandshakeOperation* op) { static bool no_suspend_no_async_exception_filter(HandshakeOperation* op) { return !op->is_suspend() && !op->is_async_exception(); } +static bool all_ops_filter(HandshakeOperation* op) { + return true; +} + +HandshakeState::HandshakeState(JavaThread* target) : + _handshakee(target), + _queue(), + _lock(Monitor::nosafepoint, "HandshakeState_lock"), + _active_handshaker(), + _async_exceptions_blocked(false), + _suspended(false), + _async_suspend_handshake(false) { +} + +HandshakeState::~HandshakeState() { + while (has_operation()) { + HandshakeOperation* op = _queue.pop(all_ops_filter); + guarantee(op->is_async(), "Only async operations may still be present on queue"); + delete op; + } +} + +void HandshakeState::add_operation(HandshakeOperation* op) { + // Adds are done lock free and so is arming. + _queue.push(op); + SafepointMechanism::arm_local_poll_release(_handshakee); +} + +bool HandshakeState::operation_pending(HandshakeOperation* op) { + MutexLocker ml(&_lock, Mutex::_no_safepoint_check_flag); + MatchOp mo(op); + return _queue.contains(mo); +} HandshakeOperation* HandshakeState::get_op_for_self(bool allow_suspend, bool check_async_exception) { assert(_handshakee == Thread::current(), "Must be called by self"); diff --git a/src/hotspot/share/runtime/handshake.hpp b/src/hotspot/share/runtime/handshake.hpp index 8012f225c65..321e691651b 100644 --- a/src/hotspot/share/runtime/handshake.hpp +++ b/src/hotspot/share/runtime/handshake.hpp @@ -126,6 +126,7 @@ class HandshakeState { public: HandshakeState(JavaThread* thread); + ~HandshakeState(); void add_operation(HandshakeOperation* op); diff --git a/src/hotspot/share/runtime/thread.cpp b/src/hotspot/share/runtime/thread.cpp index 2c698e65f38..006df6d1f7f 100644 --- a/src/hotspot/share/runtime/thread.cpp +++ b/src/hotspot/share/runtime/thread.cpp @@ -1670,9 +1670,13 @@ class InstallAsyncExceptionHandshake : public HandshakeClosure { public: InstallAsyncExceptionHandshake(AsyncExceptionHandshake* aeh) : HandshakeClosure("InstallAsyncException"), _aeh(aeh) {} + ~InstallAsyncExceptionHandshake() { + delete _aeh; + } void do_thread(Thread* thr) { JavaThread* target = JavaThread::cast(thr); target->install_async_exception(_aeh); + _aeh = nullptr; } }; diff --git a/test/hotspot/jtreg/runtime/Thread/StopAtExit.java b/test/hotspot/jtreg/runtime/Thread/StopAtExit.java index 86771cf9496..7f887c3c448 100644 --- a/test/hotspot/jtreg/runtime/Thread/StopAtExit.java +++ b/test/hotspot/jtreg/runtime/Thread/StopAtExit.java @@ -23,7 +23,7 @@ /** * @test - * @bug 8167108 8266130 8282704 8283467 + * @bug 8167108 8266130 8282704 8283467 8284632 * @summary Stress test JVM/TI StopThread() at thread exit. * @requires vm.jvmti * @modules java.base/java.lang:open @@ -93,7 +93,6 @@ public class StopAtExit extends Thread { // the thread is terminated. ThreadGroup myTG = new ThreadGroup("myTG-" + count); myTG.setDaemon(true); - Throwable myException = new ThreadDeath(); int retCode; StopAtExit thread = new StopAtExit(myTG, null); thread.start(); @@ -103,7 +102,20 @@ public class StopAtExit extends Thread { // Tell the worker thread to race to the exit and the // JVM/TI StopThread() calls will come in during thread exit. thread.exitSyncObj.countDown(); + long inner_count = 0; while (true) { + inner_count++; + + // Throw RuntimeException before ThreadDeath since a + // ThreadDeath can also be queued up when there's already + // a non-ThreadDeath async execution queued up. + Throwable myException; + if ((inner_count % 1) == 1) { + myException = new RuntimeException(); + } else { + myException = new ThreadDeath(); + } + retCode = stopThread(thread, myException); if (retCode == JVMTI_ERROR_THREAD_NOT_ALIVE) { @@ -139,7 +151,7 @@ public class StopAtExit extends Thread { } // This JVM/TI StopThread() happens after the join() so it // should do nothing, but let's make sure. - retCode = stopThread(thread, myException); + retCode = stopThread(thread, new ThreadDeath()); if (retCode != JVMTI_ERROR_THREAD_NOT_ALIVE) { throw new RuntimeException("thread " + thread.getName()