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

[Daniel D. Daugherty]

On 5/3/16 5:39 PM, 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/

src/os/solaris/vm/os_solaris.cpp
     No comments. (I'm guessing you didn't want to expand the existing
     guarantee() to cover your additional discovery.)

src/share/vm/gc/parallel/gcTaskThread.cpp
     No comments.

src/share/vm/gc/shared/concurrentGCThread.cpp
     No comments.

src/share/vm/gc/shared/workgroup.cpp
     No comments.

src/share/vm/runtime/thread.cpp
     L1388:   if (watcher != NULL)
     L1389:     delete watcher;
         nit: Please add '{' and '}' or make it a single line if-statement.

src/share/vm/runtime/vmThread.cpp
     No comments.


Thumbs up. Only one nit so feel free to ignore it or fix it; I don't
need another webrev if you fix it.

Dan


>
> 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