On 10/26/18 10:33 AM, Robbin Ehn wrote:
Hi, please review.
When the VM thread executes a handshake it uses different ThreadsLists
during
the execution. A JavaThread that is armed for the handshake when it is
already
in the exit path in VM will cancel the handshake. Even if the VM
thread cannot
see this thread after the initial ThreadsList which where used for
arming, the
handshake can progress when the exiting thread cancels the handshake.
But if a third thread takes a ThreadsList where the exiting JavaThread
is present and tries to execute a VM operation, hence waiting on VM
thread to finish the handshake, the JavaThread in the exit path can
never reach the handshake cancellation point. VM thread cannot
finishes the handshake and the third thread is stuck waiting on the VM
thread.
To allow holding a ThreadsList when executing a VM operation we
instead let the
VM thread use the same ThreadsList over the entire handshake making
all armed
threads visible to the VM thread at all time. And if VM thread spots a
terminated thread it will count that thread is already done by only
clearing
it's operation.
Passes local stress testing, t1-5 and the deadlock is no longer
reproduce-able.
Added a jtreg handshake + thread suspend test as a reproducer.
Issue: https://bugs.openjdk.java.net/browse/JDK-8212933
Code: http://cr.openjdk.java.net/~rehn/8212933/v1/webrev/
src/hotspot/share/runtime/handshake.hpp
No comments.
src/hotspot/share/runtime/handshake.cpp
L358: void HandshakeState::process_by_vmthread(JavaThread* target) {
L359: assert(Thread::current()->is_VM_thread(), "should call from
vm thread");
Both calls to handshake_process_by_vmthread() which calls this
function are made with the Threads_lock held:
MutexLockerEx ml(Threads_lock, Mutex::_no_safepoint_check_flag);
Looks like the lock is grabbed because of
possibly_vmthread_can_process_handshake() which asserts:
L351: // An externally suspended thread cannot be resumed
while the
L352: // Threads_lock is held so it is safe.
L353: // Note that this method is allowed to produce false
positives.
L354: assert(Threads_lock->owned_by_self(), "Not holding
Threads_lock.");
L355: if (target->is_ext_suspended()) {
L356: return true;
L357: }
Also looks like vmthread_can_process_handshake() needs the
Threads_lock for the same externally suspended thread check.
So I was going to ask that you add:
assert(Threads_lock->owned_by_self(), "Not holding Threads_lock.");
after L359, but how about a comment instead:
// Threads_lock must be held here, but that is assert()ed in
// possibly_vmthread_can_process_handshake().
src/hotspot/share/runtime/thread.hpp
No comments.
src/hotspot/share/runtime/thread.cpp
No comments.
src/hotspot/share/runtime/threadSMR.cpp
No comments.
test/hotspot/jtreg/runtime/handshake/HandshakeWalkSuspendExitTest.java
Very nice test! It specifically exercises ThreadLocalHandshakes
with JavaThread suspend/resume. runtime/Thread/SuspendAtExit.java
only ran into this bug by accident (JDK-8212152) so I like the
targeted test.
L49: while(!exit_now) {
nit - please add a space before '('
L51: for (int i = 0; i < _threads.length; i+=2) {
L58: for (int i = 0; i < _threads.length; i+=2) {
nit - please added spaces around '+='
So why every other thread? A comment would be good...
L52: wb.handshakeWalkStack(null, true);
I'm guessing the 'null' parameter means current thread, but
that's a guess on my part. A comment would be good.
L82: for (int i = 0; i < _threads.length; i++) {
L83: _threads[i].join();
L84: }
Thanks for cleaning up the test_threads. That will make
the JTREG thread sweeper happy. However, you don't save
the test_exit_thread references and you don't clean those
up either. Yes, I realize that they are supposed to exit,
but if something hangs up on exit, I'd rather have a join()
hang failure in this test's code than have the JTREG thread
sweeper catch it.
Dan
Thanks, Robbin
