RFR: 8268902: Testing for threadObj != NULL is unnecessary in handshake

[Coleen Phillimore]

On Wed, 16 Jun 2021 17:00:03 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> src/hotspot/share/runtime/handshake.cpp line 629:
>> 
>>> 627:   assert(_handshakee->threadObj() != NULL,
>>> 628:          "null threadobj is impossible because we're handshaking while the thread is being created");
>>> 629:   if (_handshakee->is_exiting()) {
>> 
>> The assert() is racy when it comes before the `_handshakee->is_exiting()` check.
>> It is possible for the target thread (`_handshakee`) to set the exiting condition:
>> 
>> src/hotspot/share/runtime/thread.cpp: JavaThread::exit():
>> 
>>     set_terminated(_thread_exiting);
>> 
>> and call:
>> 
>>   ensure_join(this);
>> 
>> which clears the `_threadObj` field while the caller has just
>> called `HandshakeState::suspend_with_handshake()`.
>> The caller would then fail the assert() when racing with the
>> exiting thread when what you want it to do is detect the
>> exiting condition and return false.
>
> Update: After re-reading more of the code, I think I have to change
> my comment here. The code above (`HandshakeState::suspend_with_handshake()`)
> is called from SuspendThreadHandshake::do_thread() which is a synchronous
> handshake. The caller requesting the synchronous handshake cannot proceed until
> the target thread is handshakeable. When the target thread is in `JavaThread::exit()`
> it is in thread state `_thread_in_vm` and that's not a safe/handshakeable state so
> the calling thread must wait for the target thread to become handshakeable or for
> the exiting condition to be true. No race here is possible.

Yes, the _handshakee thread - or target thread can't do suspend_with_handshake unless it's in a safe state, so the ensure_join() function must have completed or not been called.  Also ensure_join() clears the ThreadObj's Thread field but the JavaThread->_threadObj field isn't cleared:

  java_lang_Thread::set_thread(threadObj(), NULL);

The _threadObj field isn't cleared until the JavaThread* destructor so this code should not have a reference to JavaThread at this point.

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

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