RFR: 8286830: ~HandshakeState should not touch oops

[Patricio Chilano Mateo]

On Fri, 3 Jun 2022 20:34:36 GMT, Daniel D. Daugherty <dcubed 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
>
> src/hotspot/share/runtime/thread.cpp line 1367:
> 
>> 1365: void JavaThread::exit(bool destroy_vm, ExitType exit_type) {
>> 1366:   assert(this == JavaThread::current(), "thread consistency check");
>> 1367:   assert(!is_exiting(), "should not be exiting or terminated already");
> 
> Hmmm... The old `destroy_vm == true` branch assert was:
> 
> `assert(!is_terminated() && !is_exiting(), "must not be exiting");`
> 
> and the new assert mesg has "or terminated", but the assert is only
> checking `!is_exiting()` and doesn't not include `!is_terminated()`.
> So the old assert checked both conditions and only mumbled about
> "exiting" and the new assert checks one condition and mumbles
> about both conditions.

is_exiting() also checks is_terminated() internally, so it returns true for the _thread_exiting, _thread_terminated and _vm_exited cases (false only for the _not_terminated case). Since the extra !is_terminated() check was redundant I removed it. Also since the assert will trigger for both the exiting and terminated cases I added both to the assert msg.

> src/hotspot/share/runtime/thread.cpp line 1444:
> 
>> 1442:   // Also, no more async exceptions will be added to the queue after this point.
>> 1443:   set_terminated(_thread_exiting);
>> 1444:   ThreadService::current_thread_exiting(this, is_daemon(threadObj()));
> 
> Previously, we didn't call `ThreadService::current_thread_exiting()` in the
> `destroy_vm == true` branch. Is that going to result in some change in
> behavior?

That call decrements a counter used for some stats and for determining the size of the thread array in ThreadListEnumerator. But I don't see any issues on doing that since this thread is actually exiting.

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

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