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

[David Holmes]

Hi Dan,

Thanks for the Review!

On 6/05/2016 8:03 AM, Daniel D. Daugherty wrote:
> 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.)

There's no way to ask "did you use to be the WatcherThread?" because by 
this point watcherThread() has to return NULL. So simpler to just delete 
- plus this was the only OS that did this check.

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

Will add braces.

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

Thanks,
David

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