RFR: 8284632: runtime/Thread/StopAtExit.java possibly leaking memory again

[Daniel D.Daugherty]

On Wed, 27 Apr 2022 17:24:16 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> src/hotspot/share/runtime/handshake.cpp line 325:
>> 
>>> 323:     if (_handshake_cl->is_async_installer()) {
>>> 324:       _handshake_cl->do_cleanup();
>>> 325:     }
>> 
>> Rather than defining this is_async_installer() I would just add a is_exiting() check in JavaThread::install_async_exception(), like we do with suspend_with_handshake(), so we never install an async exception if the JT already moved to the _thread_exiting state. This !thread->is_terminated() check still succeeds if the thread moved to the _thread_exiting state, so we could still install an async exception between the JT moving to _thread_exiting and moving to _thread_terminated (the JT might block when trying to grab the Threads_lock to remove itself from the thread list). So in that case, we would still leak memory. (Maybe we should just check for _not_terminated here, not sure why we allow executing the handshake closure when thread is already exiting).
>> 
>> To avoid future issues I would define ~HandshakeState() with a single debug line: assert(!has_operation(), "leaking memory, queue should be empty"). If you add the following code to StopAtExit.java this assert will fire do to what I mentioned about the !thread->is_terminated() check still succeeding between _thread_exiting and _thread_terminated:
>> 
>> 
>> // Fire-up thread that just creates new threads contending on Threads_lock
>> 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();
>
> @pchilano - Thanks for reviewing! I'll have to mull on your feedback and
> then, of course, there will a long stress test cycle needed to verify.

It appears that adding an `is_exiting()` check to `JavaThread::install_async_exception()`
is insufficient. I restored my debug counter code and disabled the two leak fixes in this
PR and retested:


vm_exit: dcubed_async_global_alloc_cnt=936344
vm_exit: dcubed_drop_installer_thread_terminated_cnt=113472
vm_exit: dcubed_drop_is_exiting_cnt=13
vm_exit: dcubed_async_blocked_cnt=67577
dcubed_async_global_release_cnt=755282


dcubed_async_global_alloc_cnt=936344 - dcubed_async_global_release_cnt=755282
181062 // The number of exceptions leaked.

181062 - dcubed_drop_is_exiting_cnt=13
181049 // The number of leaked exceptions that made it past the is_exiting() check.

181049 - dcubed_drop_installer_thread_terminated_cnt=113472
67577 // The number of exceptions that made it past the is_exiting() check and
           // the is_terminated() check in HandshakeOperation::do_handshake().

67577 - dcubed_async_blocked_cnt=67577
0   // The number of exception that made it past the cleanup in JavaThread::exit

-------------

PR: https://git.openjdk.java.net/jdk/pull/8388