RFR: 8286830: ~HandshakeState should not touch oops

[Robbin Ehn]

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.

The state which hides it from the rest of the VM is the removal from the ThreadsList.
Since the JT after the state "is_exiting" still uses oops and interacts with Java, it's no where close to be hidden.
It participates in safepoint, still does JVM TI callbacks, it generates JFR events, etc.. (I guess posting contended monitor enter after posting thread end is a bug).
The basically the only subsystem that care about terminated flag is JVM TI and other 'serviceability'.
You can see the comment: "These things needs to be done while we are still a Java Thread"
And while we are a JavaThread we must participate in safepoints and handshakes.

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

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