JDK-8231032 : ThreadMXBean locking tests fail after JSR 166 refresh
  • Type: Bug
  • Component: core-libs
  • Sub-Component: java.util.concurrent
  • Affected Version: 14
  • Priority: P3
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2019-09-15
  • Updated: 2019-10-08
  • Resolved: 2019-09-27
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 14
14 b17Fixed
Related Reports
Relates :  
JDK-8229442 - AQS and lock classes refresh
Relates :  
JDK-8231036 - vmTestbase monitoring tests fail after JSR 166 refresh
Sub Tasks
JDK-8231034 :  
ProblemList failing ThreadMXBean tests - Resolved
Description
After JDK-8229442 some ThreadMXBean tests fail  because they check that the blocker object of a condition wait is a AbstractQueuedSynchronizer$ConditionObject, (not a ConditionNode).

- java/lang/management/ThreadMXBean/SynchronizerLockingThread.java

JavaTest Message: Test threw exception: java.lang.RuntimeException: LockInfo : java.util.concurrent.locks.AbstractQueuedSynchronizer$ConditionNode@393b28bb class name not matched. Expected: java.util.concurrent.locks.AbstractQueuedSynchronizer$ConditionObject
Comments
URL: https://hg.openjdk.java.net/jdk/jdk/rev/489532b89775 User: martin Date: 2019-09-27 19:31:11 +0000
27-09-2019
A contributing cause was failure to use the new LockSupport.setCurrentBlocker properly (the park call was overwriting the record of the public ConditionObject). Now fixed.
21-09-2019
The isSuspended probably also returns true if suspended via the debugger API. I'll try to find cycles to determine exactly what LockInfo examines and how we can fix this.
17-09-2019
I noticed ThreadInfo#isSuspended Presumably that always returns false, and the doc should be updated? https://download.java.net/java/early_access/jdk14/docs/api/java.management/java/lang/management/ThreadInfo.html#isSuspended()
17-09-2019
I agree with David. The identity of the park blocker is exposed to users though the monitoring API. We can change that but we should have a good reason. ConditionNode is worse than ConditionObject because - It's not public - It's not a Condition We could engineer our Condition blockers to be more meaningful. Imagine if Conditions had a user-meaningful name. Then instead of private final Condition notEmpty = takeLock.newCondition(); we might have private final Condition notEmpty = takeLock.newCondition("LinkedBlockingQueue not empty"); and that string would appear in the ThreadInfo somewhere.
17-09-2019
The only thing LockInfo can query is whether an object is an instance of Condition or AbstractOwnableSynchronizer. Seems to me whatever objects pass that test are the objects that LockInfo should be reporting. ConditionNode is not a Condition. AFAICS ManagedBlocker plays not part in the LockInfo API so that is not what should be used for reporting here.
17-09-2019
A public API of a lock could be defined as an interface or abstract class. Then the real object is implemented in an internal class. The LockInfo::getClassName has no correlation on which public API the lock object is obtained.
17-09-2019
I disagree. According to the specification: public class LockInfo extends Object Information about a lock. A lock can be a built-in object monitor, an ownable synchronizer, or the Condition object associated with synchronizers. --- That says to me that there must be correlation between the application-level locking object (as returned from newCondition()) and the locking object reported by LockInfo. So to me LockInfo needs to report the ConditionObject not the internal implementation detail that is a ConditionNode. Perhaps the past correlation was just accidental, but it doesn't seem that useful to me to have a management/monitoring API that returns internal implementation details that can't be readily mapped back to what is seen in the user's code.
16-09-2019
java.lang.management.LockInfo represents a lock object for monitoring by the class name and the identity hashcode. The true lock object after JDK-8229442 is ConditionNode. LockedSynchronizers test creates a lock by calling ReentrantLock::newCondition but SynchronizerLockingThread::checkThreadInfo assumes that ConditionObject is the object that the thread is blocked on. LockInfo::getClassName returning the class name of ConditionNode is correct. The test needs update for JDK-8229442 to relax the check of exact match of the class name. My suggestion is to check LockInfo::getClassName returned name starts with "java.util.concurrent.locks.AbstractQueuedSynchronizer"
16-09-2019
Below is a weird program to print out ThreadInfo of thread waiting to acquire, or waiting on condition ----- import java.lang.management.*; import java.util.concurrent.locks.*; public class Blockers { static void printThreadInfo(Thread thread) { System.out.print(ManagementFactory.getThreadMXBean().getThreadInfo(thread.getId())); } public static void main(String[] args) throws Throwable { final ReentrantLock lock = new ReentrantLock(); final Condition condition = lock.newCondition(); final Thread thread = new Thread(() -> { try { lock.lock(); condition.await(); } catch (InterruptedException expected) {} }); lock.lock(); thread.start(); while (!lock.hasQueuedThreads() || thread.getState() != Thread.State.WAITING) Thread.yield(); printThreadInfo(thread); lock.unlock(); for (;;) { lock.lock(); try { if (lock.hasWaiters(condition) && thread.getState() == Thread.State.WAITING) break; Thread.yield(); } finally { lock.unlock(); } } printThreadInfo(thread); thread.interrupt(); thread.join(); } } ----- With jdk13 it prints "Thread-0" prio=5 Id=12 WAITING on java.util.concurrent.locks.ReentrantLock$NonfairSync@258e2e41 owned by "main" Id=1 "Thread-0" prio=5 Id=12 WAITING on java.util.concurrent.locks.AbstractQueuedSynchronizer$ConditionObject@4f638935 while with latest jdk it prints "Thread-0" prio=5 Id=12 WAITING on java.util.concurrent.locks.ReentrantLock$NonfairSync@3d299e3 owned by "main" Id=1 "Thread-0" prio=5 Id=12 WAITING on java.util.concurrent.locks.AbstractQueuedSynchronizer$ConditionNode@4387b79e ---- Neither variant is as informative as one might like. Neither makes clear which lock the condition blocker belongs to. NonFairSync is not public, although it's not hard to guess it belongs to a ReentrantLock. In both cases thread state is WAITING, so this does not distinguish. Should queued acquirers be in BLOCKED instead (too late!)? ConditionObject is at least a public class, so users can tell it's a Condition implementation without reading the source code. OTOH ConditionNode is private and ... different. At least it has a good name that strongly hints this thread is waiting on a Condition. And .. oh yeah, java.lang.RuntimeException: LockInfo : java.util.concurrent.locks.AbstractQueuedSynchronizer$ConditionNode@764b7a84 class name not matched. Expected: java.util.concurrent.locks.AbstractQueuedSynchronizer$ConditionObject at SynchronizerLockingThread.checkThreadInfo(SynchronizerLockingThread.java:188) at SynchronizerLockingThread.checkLocks(SynchronizerLockingThread.java:128) at SynchronizerLockingThread.checkLocks(SynchronizerLockingThread.java:83) at LockedSynchronizers.main(LockedSynchronizers.java:56) It looks like the test is expecting the Condition being waited for to be the blocker exposed by the monitoring API, which is reasonable, even though we don't satisfy this for Lock acquire (ReentrantLock's blocker is an internal AQS, not the ReentrantLock itself)
16-09-2019
I don't understand the first proposed change. Is there something about ThreadMXBeans that ignores the recorded blocker? If so, that seems suspicious. Or is it just that the test tried to validate this with stack trace (which would indeed no longer always work).
16-09-2019
The tesst have been excluded so dropping back to P3. Note the test/jdk/ProblemList.txt will need to be updated as part of this patch.
16-09-2019
The nsk tests had non-portable brittle expectations of the stack frames while holding a lock. This one at least seems to have an easy band-aid fix: diff --git a/test/hotspot/jtreg/vmTestbase/nsk/monitoring/share/thread/SynchronizerLockingThreads.java b/test/hotspot/jtreg/vmTestbase/nsk/monitoring/share/thread/SynchronizerLockingThreads.java --- a/test/hotspot/jtreg/vmTestbase/nsk/monitoring/share/thread/SynchronizerLockingThreads.java +++ b/test/hotspot/jtreg/vmTestbase/nsk/monitoring/share/thread/SynchronizerLockingThreads.java @@ -243,7 +243,7 @@ protected boolean isStackTraceElementExpected(StackTraceElement element) { return super.isStackTraceElementExpected(element) || checkStackTraceElement(element, expectedMethodsThread2) || - element.getClassName().startsWith("java.util.concurrent.locks.") || + element.getClassName().startsWith("java.util.concurrent.") || element.getClassName().startsWith("jdk.internal.misc."); } }
16-09-2019
Here's an experimental change that seems to make java/lang/management/ThreadMXBean/ pass by restoring the blocker object to be a ConditionObject. It does have a cost - one extra field in ConditionNode. If we want to go down this road, probably instead want to make ConditionNode an inner class of ConditionObject. diff --git a/src/java.base/share/classes/java/util/concurrent/locks/AbstractQueuedSynchronizer.java b/src/java.base/share/classes/java/util/concurrent/locks/AbstractQueuedSynchronizer.java --- a/src/java.base/share/classes/java/util/concurrent/locks/AbstractQueuedSynchronizer.java +++ b/src/java.base/share/classes/java/util/concurrent/locks/AbstractQueuedSynchronizer.java @@ -491,6 +491,8 @@ static final class ConditionNode extends Node implements ForkJoinPool.ManagedBlocker { ConditionNode nextWaiter; // link to next waiting node + final Object blocker; + ConditionNode(Object blocker) { this.blocker = blocker; } /** * Allows Conditions to be used in ForkJoinPools without @@ -502,7 +504,7 @@ } public final boolean block() { - while (!isReleasable()) LockSupport.park(this); + while (!isReleasable()) LockSupport.park(blocker); return true; } } @@ -1562,7 +1564,7 @@ * </ol> */ public final void awaitUninterruptibly() { - ConditionNode node = new ConditionNode(); + ConditionNode node = new ConditionNode(this); int savedState = enableWait(node); LockSupport.setCurrentBlocker(this); // for back-compatibility boolean interrupted = false; @@ -1601,7 +1603,7 @@ public final void await() throws InterruptedException { if (Thread.interrupted()) throw new InterruptedException(); - ConditionNode node = new ConditionNode(); + ConditionNode node = new ConditionNode(this); int savedState = enableWait(node); LockSupport.setCurrentBlocker(this); // for back-compatibility boolean interrupted = false, cancelled = false; @@ -1647,7 +1649,7 @@ throws InterruptedException { if (Thread.interrupted()) throw new InterruptedException(); - ConditionNode node = new ConditionNode(); + ConditionNode node = new ConditionNode(this); int savedState = enableWait(node); long nanos = (nanosTimeout < 0L) ? 0L : nanosTimeout; long deadline = System.nanoTime() + nanos; @@ -1691,7 +1693,7 @@ long abstime = deadline.getTime(); if (Thread.interrupted()) throw new InterruptedException(); - ConditionNode node = new ConditionNode(); + ConditionNode node = new ConditionNode(this); int savedState = enableWait(node); boolean cancelled = false, interrupted = false; while (!canReacquire(node)) { @@ -1732,7 +1734,7 @@ long nanosTimeout = unit.toNanos(time); if (Thread.interrupted()) throw new InterruptedException(); - ConditionNode node = new ConditionNode(); + ConditionNode node = new ConditionNode(this); int savedState = enableWait(node); long nanos = (nanosTimeout < 0L) ? 0L : nanosTimeout; long deadline = System.nanoTime() + nanos;
16-09-2019
Ideally the monitoring info should not change, so we may want to fix j.u.c. instead. ConditionObject is a much better blocker than ConditionNode, if possible.
15-09-2019