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

[Patricio Chilano Mateo]

On Mon, 25 Apr 2022 22:12:38 GMT, Daniel D. Daugherty <dcubed 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.

Hi Dan,

Good find! Comments below.

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();

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

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