Merge pull request bndtools#5371 from bjhargrave/workspace-lock-deadl…

…ock-detection-fix workspace: Fixes to workspace lock write-upgrade detection
bjhargrave · Sep 19, 2022 · e1eefb7 · e1eefb7
2 parents 8cf83f4 + 3ca0efb
commit e1eefb7
Show file tree

Hide file tree

Showing 2 changed files with 105 additions and 14 deletions.
diff --git a/biz.aQute.bndlib.tests/test/test/WorkspaceTest.java b/biz.aQute.bndlib.tests/test/test/WorkspaceTest.java
@@ -2,6 +2,7 @@
 
 import static java.util.stream.Collectors.toSet;
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatIllegalStateException;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertTrue;
@@ -12,6 +13,7 @@
 import java.util.List;
 import java.util.Set;
 
+import org.assertj.core.api.Assertions;
 import org.junit.jupiter.api.Test;
 import org.osgi.framework.Version;
 
@@ -302,4 +304,82 @@ public void testJavacDefaults() throws Exception {
 			assertEquals(version, w.getProperty("javac.target"));
 		}
 	}
+
+	@Test
+	void workspace_lock_deadlock() throws Exception {
+		IO.copy(new File("testresources/ws"), testDir);
+		try (Workspace ws = Workspace.getWorkspace(testDir)) {
+			ws.readLocked(() -> {
+				assertThatIllegalStateException().isThrownBy(() -> {
+					ws.writeLocked(() -> {
+						Assertions.fail("Invalid upgrade from readlock to writelock");
+						return null;
+					});
+				});
+				return null;
+			});
+		}
+	}
+
+	@Test
+	void workspace_lock_deadlock_nested_readlock() throws Exception {
+		IO.copy(new File("testresources/ws"), testDir);
+		try (Workspace ws = Workspace.getWorkspace(testDir)) {
+			ws.readLocked(() -> {
+				ws.readLocked(() -> {
+					assertThatIllegalStateException().isThrownBy(() -> {
+						ws.writeLocked(() -> {
+							Assertions.fail("Invalid upgrade from readlock to writelock");
+							return null;
+						});
+					});
+					return null;
+				});
+				return null;
+			});
+		}
+	}
+
+	@Test
+	void workspace_lock_deadlock_released_readlock() throws Exception {
+		IO.copy(new File("testresources/ws"), testDir);
+		try (Workspace ws = Workspace.getWorkspace(testDir)) {
+			ws.readLocked(() -> {
+				ws.readLocked(() -> { // nested and released
+					return null;
+				});
+				assertThatIllegalStateException().isThrownBy(() -> {
+					ws.writeLocked(() -> {
+						Assertions.fail("Invalid upgrade from readlock to writelock");
+						return null;
+					});
+				});
+				return null;
+			});
+		}
+	}
+
+	@Test
+	void workspace_lock_deadlock_write_read_write() throws Exception {
+		IO.copy(new File("testresources/ws"), testDir);
+		try (Workspace ws = Workspace.getWorkspace(testDir)) {
+			ws.writeLocked(() -> {
+				ws.writeLocked(() -> {
+					ws.readLocked(() -> { // nested and released
+						return null;
+					});
+					return null;
+				});
+				return null;
+			}, (t) -> {
+				assertThatIllegalStateException().isThrownBy(() -> {
+					ws.writeLocked(() -> {
+						Assertions.fail("Invalid upgrade from readlock to writelock");
+						return null;
+					});
+				});
+				return null;
+			}, 10_000L);
+		}
+	}
 }
diff --git a/biz.aQute.bndlib/src/aQute/bnd/build/WorkspaceLock.java b/biz.aQute.bndlib/src/aQute/bnd/build/WorkspaceLock.java
@@ -5,11 +5,11 @@
 import java.lang.management.ManagementFactory;
 import java.lang.management.ThreadMXBean;
 import java.util.Arrays;
-import java.util.List;
 import java.util.Objects;
+import java.util.Queue;
 import java.util.concurrent.Callable;
 import java.util.concurrent.CancellationException;
-import java.util.concurrent.CopyOnWriteArrayList;
+import java.util.concurrent.ConcurrentLinkedQueue;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
 import java.util.concurrent.atomic.AtomicInteger;
@@ -31,7 +31,7 @@ final class WorkspaceLock extends ReentrantReadWriteLock {
 	private static final long	serialVersionUID	= 1L;
 	private static final Logger	logger				= LoggerFactory.getLogger(WorkspaceLock.class);
 	private final AtomicInteger	progress			= new AtomicInteger();
-	private final List<Thread>	readHolders			= new CopyOnWriteArrayList<>();
+	private final Queue<Thread>	readLockHolders		= new ConcurrentLinkedQueue<>();
 
 	WorkspaceLock(boolean fair) {
 		super(fair);
@@ -107,6 +107,13 @@ private TimeoutException timeout(Lock lock) {
 		return e;
 	}
 
+	private IllegalStateException deadlock(Lock lock) {
+		IllegalStateException e = new IllegalStateException(String.format(
+			"Deadlock situation detected trying to %s acquire %s. The current thread already holds a read lock: %s",
+			type(lock), this, Thread.currentThread()));
+		return e;
+	}
+
 	<T, U> T writeReadLocked(final long timeoutInMs, final Callable<U> underWrite,
 		final FunctionWithException<U, T> underRead, final BooleanSupplier canceled) throws Exception {
 		Callable<U> writeLocked = () -> {
@@ -133,15 +140,9 @@ <T, U> T writeReadLocked(final long timeoutInMs, final Callable<U> underWrite,
 
 	<T> T locked(final Lock lock, final long timeoutInMs, final Callable<T> callable, final BooleanSupplier canceled)
 		throws Exception {
-		Thread thread = Thread.currentThread();
 		boolean interrupted = Thread.interrupted();
-		boolean write = lock == writeLock();
-		if (write) {
-			if (readHolders.contains(thread))
-				throw new IllegalStateException("About to enter a deadlock situation. The thread " + thread
-					+ " that already holds a read lock attempts to acquire the workspace write lock.");
-		} else
-			readHolders.add(thread);
+		final Thread currentThread = Thread.currentThread();
+		final boolean readLockRequest = lock == readLock();
 
 		trace("Enter", lock);
 		try {
@@ -162,12 +163,24 @@ <T> T locked(final Lock lock, final long timeoutInMs, final Callable<T> callable
 				}
 				if (locked) {
 					try {
+						if (readLockRequest) {
+							readLockHolders.add(currentThread);
+							try {
+								return callable.call();
+							} finally {
+								readLockHolders.remove(currentThread);
+							}
+						}
 						return callable.call();
 					} finally {
 						progress.incrementAndGet();
 						lock.unlock();
 					}
 				}
+				// We cannot hold read lock when requesting write lock
+				if (!readLockRequest && readLockHolders.contains(currentThread)) {
+					throw deadlock(lock);
+				}
 				int currentProgress = progress.get();
 				if (startingProgress == currentProgress) {
 					long elapsed = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - start);
@@ -181,10 +194,8 @@ <T> T locked(final Lock lock, final long timeoutInMs, final Callable<T> callable
 			throw timeout(lock);
 		} finally {
 			trace("Exit", lock);
-			readHolders.remove(thread);
 			if (interrupted) {
-				Thread.currentThread()
-					.interrupt();
+				currentThread.interrupt();
 			}
 		}
 	}