From 6cc57a88e86ac04690ea3f0b1d5120d0985f454f Mon Sep 17 00:00:00 2001 From: dcubed Date: Wed, 26 Feb 2014 17:36:20 -0800 Subject: [PATCH] 8028073: race condition in ObjectMonitor implementation causing deadlocks Summary: Move redo of ParkEvent.unpark() after JVMTI_EVENT_MONITOR_WAITED event handler is called. Reviewed-by: rdurbin, acorn, sspitsyn, dsimms, dholmes --- src/share/vm/prims/jvm.cpp | 8 +++- src/share/vm/runtime/objectMonitor.cpp | 64 +++++++++++++++++++------- 2 files changed, 55 insertions(+), 17 deletions(-) diff --git a/src/share/vm/prims/jvm.cpp b/src/share/vm/prims/jvm.cpp index a757e1948..947da5f08 100644 --- a/src/share/vm/prims/jvm.cpp +++ b/src/share/vm/prims/jvm.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 1997, 2013, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 1997, 2014, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -518,6 +518,12 @@ JVM_ENTRY(void, JVM_MonitorWait(JNIEnv* env, jobject handle, jlong ms)) JavaThreadInObjectWaitState jtiows(thread, ms != 0); if (JvmtiExport::should_post_monitor_wait()) { JvmtiExport::post_monitor_wait((JavaThread *)THREAD, (oop)obj(), ms); + + // The current thread already owns the monitor and it has not yet + // been added to the wait queue so the current thread cannot be + // made the successor. This means that the JVMTI_EVENT_MONITOR_WAIT + // event handler cannot accidentally consume an unpark() meant for + // the ParkEvent associated with this ObjectMonitor. } ObjectSynchronizer::wait(obj, ms, CHECK); JVM_END diff --git a/src/share/vm/runtime/objectMonitor.cpp b/src/share/vm/runtime/objectMonitor.cpp index 523887502..4fcbf5d55 100644 --- a/src/share/vm/runtime/objectMonitor.cpp +++ b/src/share/vm/runtime/objectMonitor.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 1998, 2013, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 1998, 2014, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -382,6 +382,12 @@ void ATTR ObjectMonitor::enter(TRAPS) { DTRACE_MONITOR_PROBE(contended__enter, this, object(), jt); if (JvmtiExport::should_post_monitor_contended_enter()) { JvmtiExport::post_monitor_contended_enter(jt, this); + + // The current thread does not yet own the monitor and does not + // yet appear on any queues that would get it made the successor. + // This means that the JVMTI_EVENT_MONITOR_CONTENDED_ENTER event + // handler cannot accidentally consume an unpark() meant for the + // ParkEvent associated with this ObjectMonitor. } OSThreadContendState osts(Self->osthread()); @@ -439,6 +445,12 @@ void ATTR ObjectMonitor::enter(TRAPS) { DTRACE_MONITOR_PROBE(contended__entered, this, object(), jt); if (JvmtiExport::should_post_monitor_contended_entered()) { JvmtiExport::post_monitor_contended_entered(jt, this); + + // The current thread already owns the monitor and is not going to + // call park() for the remainder of the monitor enter protocol. So + // it doesn't matter if the JVMTI_EVENT_MONITOR_CONTENDED_ENTERED + // event handler consumed an unpark() issued by the thread that + // just exited the monitor. } if (event.should_commit()) { @@ -1456,6 +1468,14 @@ void ObjectMonitor::wait(jlong millis, bool interruptible, TRAPS) { // Note: 'false' parameter is passed here because the // wait was not timed out due to thread interrupt. JvmtiExport::post_monitor_waited(jt, this, false); + + // In this short circuit of the monitor wait protocol, the + // current thread never drops ownership of the monitor and + // never gets added to the wait queue so the current thread + // cannot be made the successor. This means that the + // JVMTI_EVENT_MONITOR_WAITED event handler cannot accidentally + // consume an unpark() meant for the ParkEvent associated with + // this ObjectMonitor. } if (event.should_commit()) { post_monitor_wait_event(&event, 0, millis, false); @@ -1499,21 +1519,6 @@ void ObjectMonitor::wait(jlong millis, bool interruptible, TRAPS) { exit (true, Self) ; // exit the monitor guarantee (_owner != Self, "invariant") ; - // As soon as the ObjectMonitor's ownership is dropped in the exit() - // call above, another thread can enter() the ObjectMonitor, do the - // notify(), and exit() the ObjectMonitor. If the other thread's - // exit() call chooses this thread as the successor and the unpark() - // call happens to occur while this thread is posting a - // MONITOR_CONTENDED_EXIT event, then we run the risk of the event - // handler using RawMonitors and consuming the unpark(). - // - // To avoid the problem, we re-post the event. This does no harm - // even if the original unpark() was not consumed because we are the - // chosen successor for this monitor. - if (node._notified != 0 && _succ == Self) { - node._event->unpark(); - } - // The thread is on the WaitSet list - now park() it. // On MP systems it's conceivable that a brief spin before we park // could be profitable. @@ -1597,6 +1602,33 @@ void ObjectMonitor::wait(jlong millis, bool interruptible, TRAPS) { JvmtiExport::post_monitor_waited(jt, this, ret == OS_TIMEOUT); } + // Without the fix for 8028280, it is possible for the above call: + // + // Thread::SpinAcquire (&_WaitSetLock, "WaitSet - unlink") ; + // + // to consume the unpark() that was done when the successor was set. + // The solution for this very rare possibility is to redo the unpark() + // outside of the JvmtiExport::should_post_monitor_waited() check. + // + if (node._notified != 0 && _succ == Self) { + // In this part of the monitor wait-notify-reenter protocol it + // is possible (and normal) for another thread to do a fastpath + // monitor enter-exit while this thread is still trying to get + // to the reenter portion of the protocol. + // + // The ObjectMonitor was notified and the current thread is + // the successor which also means that an unpark() has already + // been done. The JVMTI_EVENT_MONITOR_WAITED event handler can + // consume the unpark() that was done when the successor was + // set because the same ParkEvent is shared between Java + // monitors and JVM/TI RawMonitors (for now). + // + // We redo the unpark() to ensure forward progress, i.e., we + // don't want all pending threads hanging (parked) with none + // entering the unlocked monitor. + node._event->unpark(); + } + if (event.should_commit()) { post_monitor_wait_event(&event, node._notifier_tid, millis, ret == OS_TIMEOUT); } -- GitLab