diff --git a/biz.aQute.bndlib.tests/test/test/WorkspaceTest.java b/biz.aQute.bndlib.tests/test/test/WorkspaceTest.java index 70acdc1c4d..330b9e422b 100644 --- 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 index a77f7d2fe5..5b82746cc1 100644 --- 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 readHolders = new CopyOnWriteArrayList<>(); + private final Queue 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 writeReadLocked(final long timeoutInMs, final Callable underWrite, final FunctionWithException underRead, final BooleanSupplier canceled) throws Exception { Callable writeLocked = () -> { @@ -133,15 +140,9 @@ T writeReadLocked(final long timeoutInMs, final Callable underWrite, T locked(final Lock lock, final long timeoutInMs, final Callable 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 locked(final Lock lock, final long timeoutInMs, final Callable 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 locked(final Lock lock, final long timeoutInMs, final Callable callable throw timeout(lock); } finally { trace("Exit", lock); - readHolders.remove(thread); if (interrupted) { - Thread.currentThread() - .interrupt(); + currentThread.interrupt(); } } }