Date: Tue, 30 May 2023 15:42:59 +0800
Subject: [PATCH 03/59] 8187408: AbstractQueuedSynchronizer wait queue corrupted when thread awaits without holding the lock
Bug url: https://bugs.openjdk.org/browse/JDK-8187408
---
.../locks/AbstractQueuedLongSynchronizer.java | 20 +--
.../locks/AbstractQueuedSynchronizer.java | 52 +++---
jdk/test/java/util/Bug8187408/Bug8187408.java | 165 ++++++++++++++++++
3 files changed, 207 insertions(+), 30 deletions(-)
create mode 100644 jdk/test/java/util/Bug8187408/Bug8187408.java
diff --git a/jdk/src/share/classes/java/util/concurrent/locks/AbstractQueuedLongSynchronizer.java b/jdk/src/share/classes/java/util/concurrent/locks/AbstractQueuedLongSynchronizer.java
index 8699fc9b8..5adc39e17 100644
--- a/jdk/src/share/classes/java/util/concurrent/locks/AbstractQueuedLongSynchronizer.java
+++ b/jdk/src/share/classes/java/util/concurrent/locks/AbstractQueuedLongSynchronizer.java
@@ -64,11 +64,11 @@ public abstract class AbstractQueuedLongSynchronizer
private static final long serialVersionUID = 7373984972572414692L;
/*
- To keep sources in sync, the remainder of this source file is
- exactly cloned from AbstractQueuedSynchronizer, replacing class
- name and changing ints related with sync state to longs. Please
- keep it that way.
- */
+ * To keep sources in sync, the remainder of this source file is
+ * exactly cloned from AbstractQueuedSynchronizer, replacing class
+ * name and changing ints related with sync state to longs. Please
+ * keep it that way.
+ */
/**
* Creates a new {@code AbstractQueuedLongSynchronizer} instance
@@ -946,8 +946,7 @@ public abstract class AbstractQueuedLongSynchronizer
/**
* Returns {@code true} if synchronization is held exclusively with
* respect to the current (calling) thread. This method is invoked
- * upon each call to a non-waiting {@link ConditionObject} method.
- * (Waiting methods instead invoke {@link #release}.)
+ * upon each call to a {@link ConditionObject} method.
*
* <p>The default implementation throws {@link
* UnsupportedOperationException}. This method is invoked
@@ -1597,9 +1596,8 @@ public abstract class AbstractQueuedLongSynchronizer
}
/**
- * Condition implementation for a {@link
- * AbstractQueuedLongSynchronizer} serving as the basis of a {@link
- * Lock} implementation.
+ * Condition implementation for a {@link AbstractQueuedLongSynchronizer}
+ * serving as the basis of a {@link Lock} implementation.
*
* <p>Method documentation for this class describes mechanics,
* not behavioral specifications from the point of view of Lock
@@ -1632,6 +1630,8 @@ public abstract class AbstractQueuedLongSynchronizer
* @return its new wait node
*/
private Node addConditionWaiter() {
+ if (!isHeldExclusively())
+ throw new IllegalMonitorStateException();
Node t = lastWaiter;
// If lastWaiter is cancelled, clean out.
if (t != null && t.waitStatus != Node.CONDITION) {
diff --git a/jdk/src/share/classes/java/util/concurrent/locks/AbstractQueuedSynchronizer.java b/jdk/src/share/classes/java/util/concurrent/locks/AbstractQueuedSynchronizer.java
index 9088e5894..0e2bcdfef 100644
--- a/jdk/src/share/classes/java/util/concurrent/locks/AbstractQueuedSynchronizer.java
+++ b/jdk/src/share/classes/java/util/concurrent/locks/AbstractQueuedSynchronizer.java
@@ -192,19 +192,13 @@ import sun.misc.Unsafe;
* represent the locked state. While a non-reentrant lock
* does not strictly require recording of the current owner
* thread, this class does so anyway to make usage easier to monitor.
- * It also supports conditions and exposes
- * one of the instrumentation methods:
+ * It also supports conditions and exposes some instrumentation methods:
*
* <pre> {@code
* class Mutex implements Lock, java.io.Serializable {
*
* // Our internal helper class
* private static class Sync extends AbstractQueuedSynchronizer {
- * // Reports whether in locked state
- * protected boolean isHeldExclusively() {
- * return getState() == 1;
- * }
- *
* // Acquires the lock if state is zero
* public boolean tryAcquire(int acquires) {
* assert acquires == 1; // Otherwise unused
@@ -218,14 +212,27 @@ import sun.misc.Unsafe;
* // Releases the lock by setting state to zero
* protected boolean tryRelease(int releases) {
* assert releases == 1; // Otherwise unused
- * if (getState() == 0) throw new IllegalMonitorStateException();
+ * if (!isHeldExclusively())
+ * throw new IllegalMonitorStateException();
* setExclusiveOwnerThread(null);
* setState(0);
* return true;
* }
*
+ * // Reports whether in locked state
+ * public boolean isLocked() {
+ * return getState() != 0;
+ * }
+ *
+ * public boolean isHeldExclusively() {
+ * // a data race, but safe due to out-of-thin-air guarantees
+ * return getExclusiveOwnerThread() == Thread.currentThread();
+ * }
+ *
* // Provides a Condition
- * Condition newCondition() { return new ConditionObject(); }
+ * public Condition newCondition() {
+ * return new ConditionObject();
+ * }
*
* // Deserializes properly
* private void readObject(ObjectInputStream s)
@@ -238,12 +245,17 @@ import sun.misc.Unsafe;
* // The sync object does all the hard work. We just forward to it.
* private final Sync sync = new Sync();
*
- * public void lock() { sync.acquire(1); }
- * public boolean tryLock() { return sync.tryAcquire(1); }
- * public void unlock() { sync.release(1); }
- * public Condition newCondition() { return sync.newCondition(); }
- * public boolean isLocked() { return sync.isHeldExclusively(); }
- * public boolean hasQueuedThreads() { return sync.hasQueuedThreads(); }
+ * public void lock() { sync.acquire(1); }
+ * public boolean tryLock() { return sync.tryAcquire(1); }
+ * public void unlock() { sync.release(1); }
+ * public Condition newCondition() { return sync.newCondition(); }
+ * public boolean isLocked() { return sync.isLocked(); }
+ * public boolean isHeldByCurrentThread() {
+ * return sync.isHeldExclusively();
+ * }
+ * public boolean hasQueuedThreads() {
+ * return sync.hasQueuedThreads();
+ * }
* public void lockInterruptibly() throws InterruptedException {
* sync.acquireInterruptibly(1);
* }
@@ -1168,8 +1180,7 @@ public abstract class AbstractQueuedSynchronizer
/**
* Returns {@code true} if synchronization is held exclusively with
* respect to the current (calling) thread. This method is invoked
- * upon each call to a non-waiting {@link ConditionObject} method.
- * (Waiting methods instead invoke {@link #release}.)
+ * upon each call to a {@link ConditionObject} method.
*
* <p>The default implementation throws {@link
* UnsupportedOperationException}. This method is invoked
@@ -1819,9 +1830,8 @@ public abstract class AbstractQueuedSynchronizer
}
/**
- * Condition implementation for a {@link
- * AbstractQueuedSynchronizer} serving as the basis of a {@link
- * Lock} implementation.
+ * Condition implementation for a {@link AbstractQueuedSynchronizer}
+ * serving as the basis of a {@link Lock} implementation.
*
* <p>Method documentation for this class describes mechanics,
* not behavioral specifications from the point of view of Lock
@@ -1852,6 +1862,8 @@ public abstract class AbstractQueuedSynchronizer
* @return its new wait node
*/
private Node addConditionWaiter() {
+ if (!isHeldExclusively())
+ throw new IllegalMonitorStateException();
Node t = lastWaiter;
// If lastWaiter is cancelled, clean out.
if (t != null && t.waitStatus != Node.CONDITION) {
diff --git a/jdk/test/java/util/Bug8187408/Bug8187408.java b/jdk/test/java/util/Bug8187408/Bug8187408.java
new file mode 100644
index 000000000..fee6c730d
--- /dev/null
+++ b/jdk/test/java/util/Bug8187408/Bug8187408.java
@@ -0,0 +1,165 @@
+/*
+ * Copyright (c) 2023, 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
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+ /*
+ * @test
+ * @bug 8187408
+ * @summary AbstractQueuedSynchronizer wait queue corrupted when thread awaits without holding the lock
+ */
+
+import static java.util.concurrent.TimeUnit.MILLISECONDS;
+import java.util.ArrayList;
+import java.util.Date;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ThreadLocalRandom;
+import java.util.concurrent.locks.Condition;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+public class Bug8187408 {
+ static final long LONG_DELAY_MS = 10000L;
+
+ public static void main(String[] args) throws InterruptedException {
+ final ThreadLocalRandom rnd = ThreadLocalRandom.current();
+ final AwaitMethod awaitMethod = randomAwaitMethod();
+ final int nThreads = rnd.nextInt(2, 10);
+ final ReentrantLock lock = new ReentrantLock();
+ final Condition cond = lock.newCondition();
+ final CountDownLatch done = new CountDownLatch(nThreads);
+ final ArrayList<Thread> threads = new ArrayList<>();
+
+ Runnable rogue = () -> {
+ while (done.getCount() > 0) {
+ try {
+ // call await without holding lock?!
+ await(cond, awaitMethod);
+ throw new AssertionError("should throw");
+ }
+ catch (IllegalMonitorStateException success) {}
+ catch (Throwable fail) { threadUnexpectedException(fail); }}};
+ Thread rogueThread = new Thread(rogue, "rogue");
+ threads.add(rogueThread);
+ rogueThread.start();
+
+ Runnable waiter = () -> {
+ lock.lock();
+ try {
+ done.countDown();
+ cond.await();
+ } catch (Throwable fail) {
+ threadUnexpectedException(fail);
+ } finally {
+ lock.unlock();
+ }};
+ for (int i = 0; i < nThreads; i++) {
+ Thread thread = new Thread(waiter, "waiter");
+ threads.add(thread);
+ thread.start();
+ }
+
+ assertTrue(done.await(LONG_DELAY_MS, MILLISECONDS));
+ lock.lock();
+ try {
+ assertEquals(nThreads, lock.getWaitQueueLength(cond));
+ } finally {
+ cond.signalAll();
+ lock.unlock();
+ }
+ for (Thread thread : threads) {
+ thread.join(LONG_DELAY_MS);
+ assertFalse(thread.isAlive());
+ }
+ }
+
+ private static void assertTrue(boolean expr) {
+ if (!expr)
+ throw new RuntimeException("assertion failed");
+ }
+
+ private static void assertFalse(boolean expr) {
+ if (expr)
+ throw new RuntimeException("assertion failed");
+ }
+
+ private static void assertEquals(int i, int j) {
+ if (i != j)
+ throw new AssertionError(i + " != " + j);
+ }
+
+ /**
+ * Records the given exception using {@link #threadRecordFailure},
+ * then rethrows the exception, wrapping it in an AssertionError
+ * if necessary.
+ */
+ private static void threadUnexpectedException(Throwable t) {
+ t.printStackTrace();
+ if (t instanceof RuntimeException)
+ throw (RuntimeException) t;
+ else if (t instanceof Error)
+ throw (Error) t;
+ else
+ throw new AssertionError("unexpected exception: " + t, t);
+ }
+
+ enum AwaitMethod { await, awaitTimed, awaitNanos, awaitUntil }
+ private static AwaitMethod randomAwaitMethod() {
+ AwaitMethod[] awaitMethods = AwaitMethod.values();
+ return awaitMethods[ThreadLocalRandom.current().nextInt(awaitMethods.length)];
+ }
+
+ /**
+ * Returns a new Date instance representing a time at least
+ * delayMillis milliseconds in the future.
+ */
+ private static Date delayedDate(long delayMillis) {
+ // Add 1 because currentTimeMillis is known to round into the past.
+ return new Date(System.currentTimeMillis() + delayMillis + 1);
+ }
+
+ /**
+ * Awaits condition "indefinitely" using the specified AwaitMethod.
+ */
+ private static void await(Condition c, AwaitMethod awaitMethod)
+ throws InterruptedException {
+ long timeoutMillis = 2 * LONG_DELAY_MS;
+ switch (awaitMethod) {
+ case await:
+ c.await();
+ break;
+ case awaitTimed:
+ assertTrue(c.await(timeoutMillis, MILLISECONDS));
+ break;
+ case awaitNanos:
+ long timeoutNanos = MILLISECONDS.toNanos(timeoutMillis);
+ long nanosRemaining = c.awaitNanos(timeoutNanos);
+ assertTrue(nanosRemaining > timeoutNanos / 2);
+ assertTrue(nanosRemaining <= timeoutNanos);
+ break;
+ case awaitUntil:
+ assertTrue(c.awaitUntil(delayedDate(timeoutMillis)));
+ break;
+ default:
+ throw new AssertionError();
+ }
+ }
+}
\ No newline at end of file
--
2.22.0