Don't allow reentrant transactions.

The database's transaction lock is reentrant, meaning that a thread that's already holding the lock can acquire it again. This would allow a thread that already has a transaction in progress to start another transaction, which could cause transaction isolation issues and/or lock timeouts on the database's internal locks.

Check that the current thread isn't already holding the lock when starting a transaction.

briar-core/src/org/briarproject/db/DatabaseComponentImpl.java

@@ -59,7 +59,6 @@ import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
 import java.util.concurrent.atomic.AtomicBoolean;
-import java.util.concurrent.locks.ReadWriteLock;
 import java.util.concurrent.locks.ReentrantReadWriteLock;
 import java.util.logging.Logger;
 
@@ -80,7 +79,8 @@ class DatabaseComponentImpl<T> implements DatabaseComponent {
 	private final EventBus eventBus;
 	private final ShutdownManager shutdown;
 	private final AtomicBoolean closed = new AtomicBoolean(false);
-	private final ReadWriteLock lock = new ReentrantReadWriteLock(true);
+	private final ReentrantReadWriteLock lock =
+			new ReentrantReadWriteLock(true);
 
 	private volatile int shutdownHandle = -1;
 
@@ -119,6 +119,9 @@ class DatabaseComponentImpl<T> implements DatabaseComponent {
 	}
 
 	public Transaction startTransaction(boolean readOnly) throws DbException {
+		// Don't allow reentrant locking
+		if (lock.getReadHoldCount() > 0) throw new IllegalStateException();
+		if (lock.getWriteHoldCount() > 0) throw new IllegalStateException();
 		if (readOnly) lock.readLock().lock();
 		else lock.writeLock().lock();
 		try {

briar-tests/src/org/briarproject/db/DatabaseComponentImplTest.java

@@ -65,6 +65,7 @@ import static org.briarproject.api.transport.TransportConstants.REORDERING_WINDO
 import static org.briarproject.db.DatabaseConstants.MAX_OFFERED_MESSAGES;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.fail;
 
 public class DatabaseComponentImplTest extends BriarTestCase {
@@ -1447,4 +1448,53 @@ public class DatabaseComponentImplTest extends BriarTestCase {
 
 		context.assertIsSatisfied();
 	}
+
+	@Test(expected = IllegalStateException.class)
+	public void testCannotStartReadTransactionDuringReadTransaction()
+			throws Exception {
+		testCannotStartTransactionDuringTransaction(true, true);
+	}
+
+	@Test(expected = IllegalStateException.class)
+	public void testCannotStartWriteTransactionDuringReadTransaction()
+			throws Exception {
+		testCannotStartTransactionDuringTransaction(true, false);
+	}
+
+	@Test(expected = IllegalStateException.class)
+	public void testCannotStartReadTransactionDuringWriteTransaction()
+			throws Exception {
+		testCannotStartTransactionDuringTransaction(false, true);
+	}
+
+	@Test(expected = IllegalStateException.class)
+	public void testCannotStartWriteTransactionDuringWriteTransaction()
+			throws Exception {
+		testCannotStartTransactionDuringTransaction(false, false);
+	}
+
+	private void testCannotStartTransactionDuringTransaction(
+			boolean firstTxnReadOnly, boolean secondTxnReadOnly)
+			throws Exception {
+		Mockery context = new Mockery();
+		@SuppressWarnings("unchecked")
+		final Database<Object> database = context.mock(Database.class);
+		final ShutdownManager shutdown = context.mock(ShutdownManager.class);
+		final EventBus eventBus = context.mock(EventBus.class);
+
+		context.checking(new Expectations() {{
+			oneOf(database).startTransaction();
+			will(returnValue(new Object()));
+		}});
+
+		DatabaseComponent db = createDatabaseComponent(database, eventBus,
+				shutdown);
+
+		assertNotNull(db.startTransaction(firstTxnReadOnly));
+		try {
+			db.startTransaction(secondTxnReadOnly);
+		} finally {
+			context.assertIsSatisfied();
+		}
+	}
 }