RFR: 8286830: ~HandshakeState should not touch oops

[Patricio Chilano Mateo]

On Thu, 19 May 2022 19:17:09 GMT, Patricio Chilano Mateo <pchilanomate 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

Hi David, 

Thanks for taking a look at this.

> 
> I still need to find time to look at this in more detail. Things are definitely on the right track but I want to check the details. I still think we should be able to have a simple scheme whereby once the thread is marked as _thread_exiting then it can't engage in further handshakes and so at that point we could explicitly destroy the HandshakeState rather than special casing async exceptions.
> 
If by "can't engage in further handshakes" you mean to make sure no other handshake closure could be executed against it after moving to _thread_exiting, then yes we can do that. But the queue is lock free for inserts so there could still be some handshaker trying to add an operation to the queue after that, although the closure just won't be executed. That means we know no other ThreadSelfSuspensionHandshake or AsyncExceptionHandshake operation will be added to the queue after _thread_exiting, since those happen inside the closure of a synchronous handshake. But there could still be synchronous operations remaining or about to be added to the queue, again eventhough the closures won't be executed. In fact we could have some other async operation added to the queue after that since they don't necessarily need to be added inside another handshake, but we don't have any of those today.

> I'd also like for Dan to see this and he is still away for a while unfortunately.
> 
> > Move the setting of the _thread_exiting state to include the case where JavaThread::exit() is called with destroy_vm=true.
> 
> Why do you think this is necessary? I'm not saying it is incorrect but I'm not sure it is necessary either and the less the termination sequence is perturbed the better.
> 
We need to guarantee that after the async exception cleanup no other async exception can be installed. The key for that is the is_exiting() check I added in install_async_exception(). But for the destroy_vm=true path case we never set it, so if the exiting thread blocks while trying to acquire the Threads_lock in Threads::remove() for example, we could have a new handshake that installs an async exception. Later in Threads::destroy_vm() we would hit this same issue of touching an oop when doing "delete thread".

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

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