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

[Daniel D.Daugherty]

On Wed, 27 Apr 2022 17:08:42 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

>> Throwing async exceptions at exiting JavaThreads can leak the exception:
>> 
>> 1) HandshakeOperation::do_handshake() recognizes that the target thread
>>     might be terminated, but needs to do some cleanup when the operation
>>     is one that installs an async exception.
>> 
>> 2) JavaThread::exit() uses a NoAsyncExceptionDeliveryMark to protect the
>>     VM's call out to Thread::exit() from async exceptions, but it needs to do
>>     some cleanup when those async exceptions cannot be delivered because
>>     the target thread has logically exited.
>> 
>> Also tweaked runtime/Thread/StopAtExit.java to alternate between throwing
>> RuntimeException and ThreadDeath. The async exception queuing code accepts
>> any exception, but it recognizes ThreadDeath as special. When a target thread
>> already has a queued async ThreadDeath exception, then another exception will
>> not be queued. So the test now throws RuntimeException on odd iterations and
>> ThreadDeath on even iterations and the target thread will have at most two async
>> exceptions queued up.
>> 
>> This fix has been tested with Mach5 Tier[1-7] (about to start a Tier8) and I also ran
>> my StressWrapper_StopAtExit.java test using {release, fastdebug, and slowdebug}
>> bits for 12 hours per config. No leaks detected. Previously, release bits would
>> fail with OutOfMemoryException in ~2 minutes with StressWrapper_StopAtExit.java.
>
> 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.

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

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