RFR: 8154715: Missing destructor and/or TLS clearing calls for terminating threads

[David Holmes]

On 11/05/2016 1:42 PM, serguei.spitsyn at oracle.com wrote:
> David,
>
> It looks good and safe.

Thanks Serguei!

> Is there anything to do for the closed code as well?

No.

David

> Thanks,
> Serguei
>
>
> On 5/10/16 19:17, David Holmes wrote:
>> On 11/05/2016 6:28 AM, Brian Gardner wrote:
>>> I really like the rename of java_start to native_thread_entry, it makes
>>> things make more sense.  It looks like os_bsd.cpp and os_linux.cpp are
>>> both missing the clear_thread_current logic after thread->run().
>>
>> Doh! Thanks. That's what I get for trying to multi-task :)
>>
>> Webrev updated in place.
>>
>> Can I get another review from someone in hotspot please. Dan is
>> unfortunately (for me) away.
>>
>> Thanks,
>> David
>>
>>> + // If a thread has not deleted itself ("delete this") as part of its
>>> + // termination sequence, we have to ensure thread-local-storage is
>>> + // cleared before we actually terminate. No threads should ever be
>>> + // deleted asynchronously with respect to their termination.
>>> + if (Thread::current_or_null_safe() != NULL) {
>>> + assert(Thread::current_or_null_safe() == thread, "current thread is
>>> wrong");
>>> + thread->clear_thread_current();
>>> + }
>>> +
>>>
>>>
>>>> On May 9, 2016, at 5:33 PM, David Holmes <david.holmes at oracle.com
>>>> <mailto:david.holmes at oracle.com>> wrote:
>>>>
>>>> Okay here is version 2:
>>>>
>>>> http://cr.openjdk.java.net/~dholmes/8154715/webrev.v2/
>>>>
>>>> Lots of cosmetic changes but only a couple of functional ones:
>>>>
>>>> -  After thread->run() returns we clear the TLS by calling
>>>> clear_thread_current(), but only for threads where it has not already
>>>> been cleared - as those threads may already have been deleted so we
>>>> can't dereference 'thread'
>>>>
>>>> - No asynchronous thread deletion is permitted, and we avoid races
>>>> with VM termination. This means the VMThread no longer gets deleted -
>>>> that should not be an issue as many threads do not get deleted when
>>>> the VM terminates. I added destructors for the VMThread and
>>>> WatcherThread so anyone introducing their deletion is informed by a
>>>> guarantee(false)
>>>>
>>>>
>>>> Cosmetic changes:
>>>>
>>>> - renamed java_start to thread_native_entry (it is used by all threads
>>>> not just "java" ones, so this avoids potential confusion)
>>>>
>>>> - updated os::free_thread to always assume it works on the current
>>>> thread (and add assert to verify that)
>>>>
>>>> Thanks,
>>>> David
>>>
>