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

[Stefan Karlsson]

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.

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?

   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