RFR: 8286830: ~HandshakeState should not touch oops

[David Holmes]

On Mon, 30 May 2022 08:54:44 GMT, Robbin Ehn <rehn at openjdk.org> wrote:

>> On 8284632 we added ~HandshakeState to cleanup any potential async operations left in the handshake queue. This could trigger the release of an oophandle from its associated oop-storage when trying to delete an AsyncExceptionHandshake object. Since the exiting JT already executed on_thread_detach() this is forbidden. 
>> To fix this I used the original approach that @dcubed-ojdk posted on the 8284632 fix, which is cleaning up those operations before on_thread_detach() is executed. To make sure that no other async exception operation is added to the queue after that I did two things:
>> - Added a check in install_async_exception() to avoid installing the operation if the target already moved to the _thread_exiting state.
>> - Move the setting of the _thread_exiting state to include the case where JavaThread::exit() is called with destroy_vm=true.
>> 
>> I also added another run to tests StopAtExit.java and SuspendAtExit.java with extra contention on the Threads_lock. That increases the odds of the target being handshake safe while being in the _thread_exiting state which allows testing for more pathological cases.
>> 
>> I was able to reproduce the issue by running StopAtExit.java with -XX:+UseShenandoahGC and verified that the test now passes. I also run mach5 tiers1-3. Will run the upper tiers also and will do an additional pass after that.
>> 
>> Thanks,
>> Patricio
>
> @dholmes-ora originally the idea was that if a JT is on a ThreadsList it will respond to handshakes. Note that while on the ThreadsList it may participate in safepoints.
> Since we also touch oops after "thread is exiting" we cannot stop responding to handshake after this stage. (ensure_join)
> 
> At one point I actually had a patch which removed the terminated state all together, and instead used on or off ThreadsList, which I think is the right approach, reducing complexity.
> 
> Anyhow a way longer discussion, this seems fine for now @pchilano , thanks!

@robehn Using on/off the ThreadsList does not seem right to me. Removal from the main thread-list happens much later than the point at which handshaking a terminating thread makes no sense; and removal from all threads-lists may not occur under we get into ThreadSMR::delete. To me it makes perfect sense for a thread to have a terminating state, like is_exiting, which effectively hides it from the rest of the VM in terms of interacting with it. To all and intents and purposes such a thread has already terminated as far as most of the outside world is concerned.

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

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