RFR: 8254263: Remove special_runtime_exit_condition() check from ~ThreadInVMForHandshake()

[David Holmes]

Hi Patricio,

On 12/10/2020 2:59 pm, Patricio Chilano Mateo wrote:
> On Sun, 11 Oct 2020 22:09:30 GMT, David Holmes <dholmes at openjdk.org> wrote:
> 
>>> Hi all,
>>>
>>> Please review the following patch that removes the call to handle_special_runtime_exit_condition() in
>>> ~ThreadInVMForHandshake(). This check was added by 8223572 to prevent a JavaThread that was suspended while trying to
>>> process a handshake to transition back to java without noticing it was suspended. Since 8238761, a JavaThread executing
>>> HandshakeState::process_by_self() will never become safe. It comes from an unsafe state and remains unsafe, so it
>>> cannot be suspended during that time. Removing this check also removes one of the reasons
>>> SafepointMechanism::process_if_requested() is recursive (the other one remains SafepointSynchronize::block()). Tested
>>> in mach5, tiers 1-7.  Thanks, Patricio
>>
>> src/hotspot/share/runtime/interfaceSupport.inline.hpp line 146:
>>
>>> 144:
>>> 145:   ~ThreadInVMForHandshake() {
>>> 146:     assert(_thread->thread_state() == _thread_in_vm, "should only call when leaving VM after handshake");
>>
>> Can we also assert `!_thread->has_special_runtime_exit_condition()`? Or at least that the value of this has not changed
>> between the TIVFH constructor and destructor.
> 
> has_special_runtime_exit_condition() checks for async exceptions, external_suspend and JFR suspend. The last two can
> actually be set while we are in the handshake. It doesn't mean that the caller will consider the target suspended, but
> the _suspend_flags might be different at construction and destruction of TIVFH. 

Ah right! Oh well :)

> As for async exceptions, I realized we
> have a similar issue as the one described in 8223572 when calling handle_polling_page_exception() on a return poll:
> 
> - T calls ThreadSafepointState::handle_polling_page_exception() because another thread called T.stop() (which we
>    implement with a handshake)
> - T calls SafepointMechanism::process_if_requested(thread())
> - T calls HandshakeState::process_self_inner() and processes the handshake which calls set_pending_async_exception()
> - T returns from SafepointMechanism::process_if_requested(thread()) and goes back to java without calling
>    check_and_handle_async_exceptions() to throw the ThreadDeath() exception
> 
> I'm not sure if delaying the throw of ThreadDeath is allowed, so if we need to keep the current behaviour I can add a
> call to check_and_handle_async_exceptions():

To some extent, given it is a race, a delay is permissible, but I find 
it highly undesirable. Our biggest problems with async exceptions lately 
is the unpredictability of their actually getting propagated. So we 
should fix this.

> @@ -987,6 +987,7 @@ void ThreadSafepointState::handle_polling_page_exception() {
>       // Process pending operation
>       SafepointMechanism::process_if_requested(thread());
> +   thread()->check_and_handle_async_exceptions();

Why directly in handle_polling_page_exception() rather than restoring it 
to the ~ThreadInVMForHandshake() ?

Thanks,
David
-----

>       // restore oop result, if any
> 
> -------------
> 
> PR: https://git.openjdk.java.net/jdk/pull/577
>