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

[David Holmes]

On 7/05/2016 5:36 AM, Brian Gardner wrote:
> After applying your patches I still saw the cleanup messages from
> pthread.  After comparing my original patch to yours and doing some
> testing it looks like we missed a couple spots.
>
> The warning went away after I applied the following patches:
> http://brian.timestudybuddy.com/webrev/hotspot__clear_thread_current/webrev/hotspot/src/share/vm/gc/shared/concurrentGCThread.cpp.cdiff.html http://brian.timestudybuddy.com/webrev/hotspot__clear_thread_current/webrev/hotspot/src/share/vm/gc/cms/concurrentMarkSweepThread.cpp.cdiff.html

My patch moves that from inside terminate() to outside of it:

   78 void ConcurrentGCThread::run() {
   79   initialize_in_thread();
   80   wait_for_universe_init();
   81
   82   run_service();
   83
   84   terminate();
   85
   86   // Can't "delete this" before we terminate as other code
   87   // holds references to 'this', but we must do some cleanup
   88   // ourselves before allowing the native thread to terminate
   89
   90   ThreadLocalStorage::set_thread(NULL);
   91 }

so you should not be seeing any issue because of that.

???

David



> Brian
>
>> On May 3, 2016, at 4:39 PM, David Holmes <david.holmes at oracle.com
>> <mailto:david.holmes at oracle.com>> 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
>>
>