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
