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

[Stefan Karlsson]

Hi David,

On 06/05/16 15:38, David Holmes wrote:
> Hi Stefan,
>
> Thanks for taking a look at this.
>
> On 6/05/2016 5:02 PM, Stefan Karlsson wrote:
>> Hi David,
>>
>> I looked through the GC part of this webrev and I think the change is 
>> fine.
>>
>> However, it seems a bit error prone. If we decide to change the code to,
>> for example, terminate the  AbstractGangWorker threads, then we have to
>> remember to insert a ThreadLocalStorage::set_thread(NULL) call.
>
> That's why I added the ShouldNotReachHere()'s - if those threads start 
> terminating then we will see those hit. Perhaps a comment:
>
> ShouldNotReachHere(); // If thread terminates we have to do TLS cleanup
>
> ?

Yes, I would appreciate a comment. Though, when we add new threads, we 
need to remember to add the set_thread(NULL) call.

>
>> Could we instead add a call to ThreadLocalStorage::set_thread(NULL), or
>> maybe even Thread::clear_thread_current(), in java_start?
>>
>> static void *java_start(Thread *thread) {
>> [...]
>>   thread->initialize_thread_current();
>>
>> [...]
>>
>>   // call one more level start routine
>>   thread->run();
>>
>>   ////////// Could we call Thread::clear_thread_current(); here?
>
> Not easily. For JavaThreads we've already done "delete this" inside 
> the run() method, so we'd have to move that into java_start as well, 
> but we can only do the delete for JavaThreads not for other threads. 
> And we'd also have to change the VMThread and WatcherThread 
> termination logic because of the deletes that happen in the 
> termination thread - the "this" pointer (thread above) may no longer 
> be valid when we want to call clear_current_thread() - which is why we 
> can only do the ThreadLocalStorage::set_thread(NULL).
>
> I agree it would be a lot cleaner to have java_start do:
>
>   thread->common_initialization();
>   thread->run();
>   thread->common_cleanup();
>   delete thread;
>
> for all threads, but we'd need a lot of other changes to allow for 
> that. Otherwise we would need to note that kind of thread before 
> calling run() then switch on the thread type after run() to decide 
> what kind of cleanup is necessary and possible. I don't think that 
> would be better than just doing the "right" cleanup at the end of the 
> run() methods.

I understand that this is a bit messy, and I won't insist that we change 
this in this RFR, but without looking at this in much detail it sounds 
weird to delete the thread in run(). Couldn't this be solved by 
introducing a virtual Thread::post_run() function and do:

virtual void Thread::post_run() {
   clear_thread_current();
}

virtual void JavaThread::post_run() {
   Thread::post_run();
   delete this;
}

Thanks,
StefanK

>
> Thanks,
> David
> ------
>
>>
>>   log_info(os, thread)("Thread finished (tid: " UINTX_FORMAT ", pthread
>> id: " UINTX_FORMAT ").",
>>     os::current_thread_id(), (uintx) pthread_self());
>>
>>   return 0;
>> }
>>
>> And get rid of the explicit calls to
>> ThreadLocalStorage::set_thread(NULL) you added?
>>
>> Thanks,
>> StefanK
>>
>> On 04/05/16 01:39, David Holmes wrote:
>>> This needs attention from GC and runtime folk please.
>>>
>>> bug: https://bugs.openjdk.java.net/browse/JDK-8154715
>>> webrev: http://cr.openjdk.java.net/~dholmes/8154715/webrev/
>>>
>>> tl;dr: ensure ThreadLocalStorage::set_thread(NULL) is always called
>>> before a thread terminates.
>>>
>>> Background:
>>>
>>> Most system-related threads do not expect to explicitly terminate,
>>> except sometimes as part of VM termination. Such threads don't have
>>> their destructors called, but should.
>>>
>>> This omission came to light due to the ThreadLocalStorage changes in
>>> JDK-8132510. As part of that change we deleted the following from the
>>> termination path of the VMThread:
>>>
>>>  // Thread destructor usually does this.
>>>  ThreadLocalStorage::set_thread(NULL);
>>>
>>> The clearing of TLS seemed irrelevant to the VMThread as it primarily
>>> is used to aid in JNI attach/detach. However Brian Gardner reported:
>>>
>>> http://mail.openjdk.java.net/pipermail/bsd-port-dev/2016-February/002788.html 
>>>
>>>
>>>
>>> a problem on FreeBSD caused by this change and the interaction with
>>> the POSIX  pthread TLS destructor use introduced by JDK-8033696.
>>> Because the VMThread terminated without clearing TLS, when the
>>> TLS-destructor was called it got into a loop which ran four times (as
>>> happens on Linux) and then prints a warning to the console (which
>>> doesn't happen on Linux).
>>>
>>> This indicates we need to restore the:
>>>
>>>  ThreadLocalStorage::set_thread(NULL);
>>>
>>> but on further consideration it seems to me that this is not confined
>>> to the VMThread, and the most appropriate fix would be to always
>>> invoke the Thread destructor as a thread terminates.
>>>
>>> Solution:
>>>
>>> Further investigation shows that calling the Thread destructor in the
>>> thread as it terminates is not possible:
>>>
>>> - VMThread
>>>
>>> This is actually destroyed by the thread that terminates the VM, but
>>> that can happen after it terminates and so we still hit the TLS
>>> problem. The VMThread may be able to destroy itself today but in the
>>> past this was not possible (see existing code comment), and in the
>>> future it may also not be possible - the problem is that the Thread
>>> destructor can interact with other VM subsystems that are concurrently
>>> being torn down by the thread that is terminating the VM. In the past
>>> this was the CodeHeap. So rather than introduce something that is
>>> fragile we stick with the current scheme but restore the
>>> ThreadLocalStorage::set_thread(NULL); - note we can't access "this" at
>>> that time because it may already have been de-allocated.
>>>
>>> - WatcherThread
>>>
>>> The WatcherThread is never destroyed today but has the same problem as
>>> the VMThread. We can call the destructor from the VM termination
>>> thread (and have implemented that), but not from the WatcherThread
>>> itself. So again we just have to restore the
>>> ThreadLocalStorage::set_thread(NULL); to fix the potential TLS problem.
>>>
>>> - GC Threads
>>>
>>> There are two cases:
>>>
>>> a) GC threads that never terminate
>>>
>>> For these we don't need to do anything: we can't delete the thread as
>>> it never terminates and we don't hit the TLS problem because it never
>>> terminates. So all we will do here is add some logic to check (in
>>> NON_PRODUCT) that we do in fact never terminate.
>>>
>>> b) GC threads that can terminate
>>>
>>> Despite the fact the threads can terminate, references to those
>>> threads are stored elsewhere (WorkGangs and other places) and are not
>>> cleared as part of the termination process. Those references can be
>>> touched after the thread has terminated so we can not call the
>>> destructor at all. So again all we can do (without some major thread
>>> management reworking) is ensure that
>>> ThreadLocalStorage::set_thread(NULL); is called before the thread
>>> actually terminates
>>>
>>> Testing: JPRT
>>>          RBT - runtime nightly tests
>>>
>>> Thanks,
>>> David
>>