GP-2183 corrected potential deadlock with various DBListener callbacks

Ghidra/Framework/DB/src/main/java/db/DBHandle.java

@@ -47,6 +47,7 @@ public class DBHandle {
 	private long lastTransactionID;
 	private boolean txStarted = false;
 	private boolean waitingForNewTransaction = false;
+	private boolean reloadInProgress = false;
 
 	private long checkpointNum;
 	private long lastRecoverySnapshotId;
@@ -316,8 +317,10 @@ public class DBHandle {
 	 * Close the scratch-pad database handle if it open.
 	 */
 	public void closeScratchPad() {
-		if (scratchPad != null) {
-			scratchPad.close();
+		// use copy of scratchPad to be thread-safe
+		DBHandle scratchDbh = scratchPad;
+		if (scratchDbh != null) {
+			scratchDbh.close();
 			scratchPad = null;
 		}
 	}
@@ -330,25 +333,31 @@ public class DBHandle {
 		listenerList.add(listener);
 	}
 
-	private void dbRestored() {
+	private void notifyDbRestored() {
 		for (DBListener listener : listenerList) {
 			listener.dbRestored(this);
 		}
 	}
 
-	private void dbClosed() {
+	private void notifyDbClosed() {
 		for (DBListener listener : listenerList) {
 			listener.dbClosed(this);
 		}
 	}
 
-	private void tableAdded(Table table) {
+	private void notifyTableAdded(Table table) {
+		if (reloadInProgress) {
+			return; // squash notification during reload (e.g., undo/redo)
+		}
 		for (DBListener listener : listenerList) {
 			listener.tableAdded(this, table);
 		}
 	}
 
-	void tableDeleted(Table table) {
+	void notifyTableDeleted(Table table) {
+		if (reloadInProgress) {
+			return; // squash notification during reload (e.g., undo/redo)
+		}
 		for (DBListener listener : listenerList) {
 			listener.tableDeleted(this, table);
 		}
@@ -418,15 +427,39 @@ public class DBHandle {
 	}
 
 	/**
-	 * Terminate transaction.  If commit is false, Table instances may be added 
-	 * or removed/invalidated.
+	 * End current transaction.  If commit is false a rollback may occur followed by
+	 * {@link DBListener#dbRestored(DBHandle)} notification to listeners.
+	 * 
 	 * @param id transaction ID
-	 * @param commit if true a new checkpoint will be established, if
+	 * @param commit if true a new checkpoint will be established for active transaction, if
 	 * false all changes since the previous checkpoint will be discarded.
-	 * @return true if new checkpoint established.
+	 * @return true if new checkpoint established, false if nothing to commit
+	 * or commit parameter specified as false and active transaction is terminated with rollback.
 	 * @throws IOException if IO error occurs
 	 */
-	public synchronized boolean endTransaction(long id, boolean commit) throws IOException {
+	public boolean endTransaction(long id, boolean commit) throws IOException {
+		try {
+			return doEndTransaction(id, commit);
+		}
+		catch (DBRollbackException e) {
+			notifyDbRestored();
+		}
+		return false;
+	}
+
+	/**
+	 * End current transaction.  If <code>commit</code> is false a rollback may be perfromed.
+	 * 
+	 * @param id transaction ID
+	 * @param commit if true a new checkpoint will be established for active transaction, if
+	 * false all changes since the previous checkpoint will be discarded.
+	 * @return true if new checkpoint established.
+	 * @throws DBRollbackException if <code>commit</code> is false and active transaction was 
+	 * terminated and database rollback was performed (i.e., undo performed).
+	 * @throws IOException if IO error occurs
+	 */
+	private synchronized boolean doEndTransaction(long id, boolean commit)
+			throws DBRollbackException, IOException {
 		if (id != lastTransactionID) {
 			throw new IllegalStateException("Transaction id is not active");
 		}
@@ -440,9 +473,11 @@ public class DBHandle {
 					}
 					return false;
 				}
-				// rollback
+
+				// rollback transaction
 				bufferMgr.undo(false);
 				reloadTables();
+				throw new DBRollbackException();
 			}
 		}
 		finally {
@@ -459,9 +494,33 @@ public class DBHandle {
 		return (bufferMgr != null && !bufferMgr.atCheckpoint());
 	}
 
-	public synchronized void terminateTransaction(long id, boolean commit) throws IOException {
-		endTransaction(id, commit);
-		waitingForNewTransaction = true;
+	/**
+	 * Terminate current transaction.  If commit is false a rollback may occur followed by
+	 * {@link DBListener#dbRestored(DBHandle)} notification to listeners.  This method is very 
+	 * similar to {@link #endTransaction(long, boolean)} with the added behavior of setting the 
+	 * internal {@link DBHandle} state such that any subsequent invocations of 
+	 * {@link #checkTransaction()} will throw a {@link TerminatedTransactionException} until a new 
+	 * transaction is started.
+	 * 
+	 * @param id transaction ID
+	 * @param commit if true a new checkpoint will be established for active transaction, if
+	 * false all changes since the previous checkpoint will be discarded.
+	 * @throws IOException if IO error occurs
+	 */
+	public void terminateTransaction(long id, boolean commit) throws IOException {
+		boolean rollback = false;
+		synchronized (this) {
+			try {
+				doEndTransaction(id, commit);
+			}
+			catch (DBRollbackException e) {
+				rollback = true;
+			}
+			waitingForNewTransaction = true;
+		}
+		if (rollback) {
+			notifyDbRestored();
+		}
 	}
 
 	/**
@@ -476,16 +535,22 @@ public class DBHandle {
 	 * Undo changes made during the previous transaction checkpoint.
 	 * All upper-levels must clear table-based cached data prior to 
 	 * invoking this method.
-	 * @return true if an undo was successful
+	 * @return true if an undo was successful, else false if not allowed
 	 * @throws IOException if IO error occurs
 	 */
-	public synchronized boolean undo() throws IOException {
-		if (canUndo() && bufferMgr.undo(true)) {
-			++checkpointNum;
-			reloadTables();
-			return true;
+	public boolean undo() throws IOException {
+		boolean success = false;
+		synchronized (this) {
+			if (canUndo() && bufferMgr.undo(true)) {
+				++checkpointNum;
+				reloadTables();
+				success = true;
+			}
 		}
-		return false;
+		if (success) {
+			notifyDbRestored();
+		}
+		return success;
 	}
 
 	/**
@@ -515,16 +580,22 @@ public class DBHandle {
 	 * Moves forward by one checkpoint only.
 	 * All upper-levels must clear table-based cached data prior to 
 	 * invoking this method.
-	 * @return boolean
+	 * @return boolean if redo is successful, else false if undo not allowed
 	 * @throws IOException if IO error occurs
 	 */
-	public synchronized boolean redo() throws IOException {
-		if (canRedo() && bufferMgr.redo()) {
-			++checkpointNum;
-			reloadTables();
-			return true;
+	public boolean redo() throws IOException {
+		boolean success = false;
+		synchronized (this) {
+			if (canRedo() && bufferMgr.redo()) {
+				++checkpointNum;
+				reloadTables();
+				success = true;
+			}
 		}
-		return false;
+		if (success) {
+			notifyDbRestored();
+		}
+		return success;
 	}
 
 	/**
@@ -569,7 +640,7 @@ public class DBHandle {
 	 * Close the database and dispose of the underlying buffer manager.
 	 * Any existing recovery data will be discarded.
 	 */
-	public synchronized void close() {
+	public void close() {
 		close(false);
 	}
 
@@ -578,11 +649,15 @@ public class DBHandle {
 	 * @param keepRecoveryData true if existing recovery data should be retained or false to remove
 	 * any recovery data
 	 */
-	public synchronized void close(boolean keepRecoveryData) {
+	public void close(boolean keepRecoveryData) {
+
 		closeScratchPad();
-		if (bufferMgr != null) {
-			dbClosed();
-			bufferMgr.dispose(keepRecoveryData);
+
+		// use copy of bufferMgr to be thread-safe
+		BufferMgr mgr = bufferMgr;
+		if (mgr != null) {
+			notifyDbClosed();
+			mgr.dispose(keepRecoveryData);
 			bufferMgr = null;
 		}
 	}
@@ -844,32 +919,38 @@ public class DBHandle {
 	 */
 	private void reloadTables() throws IOException {
 
-		dbParms.refresh();
+		reloadInProgress = true;
+		try {
+			dbParms.refresh();
 
-		Hashtable<String, Table> oldTables = tables;
-		tables = new Hashtable<>();
-		TableRecord[] tableRecords = masterTable.refreshTableRecords();
-		for (TableRecord tableRecord : tableRecords) {
+			Hashtable<String, Table> oldTables = tables;
+			tables = new Hashtable<>();
+			// NOTE: master table invalidates any obsolete tables during refresh
+			TableRecord[] tableRecords = masterTable.refreshTableRecords();
+			for (TableRecord tableRecord : tableRecords) {
 
-			String tableName = tableRecord.getName();
+				String tableName = tableRecord.getName();
 
-			// Process each primary tables
-			if (tableRecord.getIndexedColumn() < 0) {
-				Table t = oldTables.get(tableName);
-				if (t == null || t.isInvalid()) {
-					oldTables.remove(tableName);
-					t = new Table(this, tableRecord);
-					tableAdded(t);
+				// Process each primary tables
+				if (tableRecord.getIndexedColumn() < 0) {
+					Table t = oldTables.get(tableName);
+					if (t == null || t.isInvalid()) {
+						oldTables.remove(tableName);
+						t = new Table(this, tableRecord);
+						notifyTableAdded(t);
+					}
+					tables.put(tableName, t);
 				}
-				tables.put(tableName, t);
-			}
 
-			// secondary table indexes
-			else if (!oldTables.containsKey(tableName)) {
-				IndexTable.getIndexTable(this, tableRecord);
+				// secondary table indexes
+				else if (!oldTables.containsKey(tableName)) {
+					IndexTable.getIndexTable(this, tableRecord);
+				}
 			}
 		}
-		dbRestored();
+		finally {
+			reloadInProgress = false;
+		}
 	}
 
 	/**
@@ -917,20 +998,23 @@ public class DBHandle {
 	 * @return new table instance
 	 * @throws IOException if IO error occurs during table creation
 	 */
-	public synchronized Table createTable(String name, Schema schema, int[] indexedColumns)
+	public Table createTable(String name, Schema schema, int[] indexedColumns)
 			throws IOException {
-		if (tables.containsKey(name)) {
-			throw new IOException("Table already exists");
-		}
-		checkTransaction();
-		Table table = new Table(this, masterTable.createTableRecord(name, schema, -1));
-		tables.put(name, table);
-		if (indexedColumns != null) {
-			for (int indexedColumn : indexedColumns) {
-				IndexTable.createIndexTable(table, indexedColumn);
+		Table table;
+		synchronized (this) {
+			if (tables.containsKey(name)) {
+				throw new IOException("Table already exists");
+			}
+			checkTransaction();
+			table = new Table(this, masterTable.createTableRecord(name, schema, -1));
+			tables.put(name, table);
+			if (indexedColumns != null) {
+				for (int indexedColumn : indexedColumns) {
+					IndexTable.createIndexTable(table, indexedColumn);
+				}
 			}
 		}
-		tableAdded(table);
+		notifyTableAdded(table);
 		return table;
 	}
 
@@ -964,19 +1048,23 @@ public class DBHandle {
 	 * @param name table name
 	 * @throws IOException if there is an I/O error or the table does not exist
 	 */
-	public synchronized void deleteTable(String name) throws IOException {
-		Table table = tables.get(name);
-		if (table == null) {
-			return;
+	public void deleteTable(String name) throws IOException {
+		Table table;
+		synchronized (this) {
+			table = tables.get(name);
+			if (table == null) {
+				return;
+			}
+			checkTransaction();
+			int[] indexedColumns = table.getIndexedColumns();
+			for (int indexedColumn : indexedColumns) {
+				table.removeIndex(indexedColumn);
+			}
+			table.deleteAll();
+			masterTable.deleteTableRecord(table.getTableNum());
+			tables.remove(name);
 		}
-		checkTransaction();
-		int[] indexedColumns = table.getIndexedColumns();
-		for (int indexedColumn : indexedColumns) {
-			table.removeIndex(indexedColumn);
-		}
-		table.deleteAll();
-		masterTable.deleteTableRecord(table.getTableNum());
-		tables.remove(name);
+		notifyTableDeleted(table);
 	}
 
 	/**
@@ -1000,9 +1088,6 @@ public class DBHandle {
 		return bufferMgr.getLowBufferCount();
 	}
 
-	/*
-	 * @see java.lang.Object#finalize()
-	 */
 	@Override
 	protected void finalize() throws Throwable {
 		close(true);

Ghidra/Framework/DB/src/main/java/db/DBListener.java

@@ -1,6 +1,5 @@
 /* ###
  * IP: GHIDRA
- * REVIEWED: YES
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -23,9 +22,9 @@ public interface DBListener {
 
 	/**
 	 * Provides notification that an undo or redo was performed.
-	 * Separate notification will be provided if tables were added/removed.
-	 * The state of the database may still be in transition and should not be accessed
-	 * by this callback method.
+	 * During the restore process {@link #tableAdded(DBHandle, Table)} and
+	 * {@link #tableDeleted(DBHandle, Table)} notifications will be supressed.
+	 * Any listener concerned with tables added or removed should reacquire their table(s).
 	 * @param dbh associated database handle
 	 */
 	void dbRestored(DBHandle dbh);

Ghidra/Framework/DB/src/main/java/db/DBRollbackException.java

@@ -0,0 +1,30 @@
+/* ###
+ * IP: GHIDRA
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * 
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package db;
+
+/**
+ * <code>DBRollbackException</code> thrown when a database transaction rollback was performed
+ * during transaction termination.
+ */
+class DBRollbackException extends Exception {
+
+	/**
+	 * Construct exception
+	 */
+	DBRollbackException() {
+		super();
+	}
+}

Ghidra/Framework/DB/src/main/java/db/Table.java

@@ -112,14 +112,10 @@ public class Table {
 	 * Subsequent table use may generate an exception.
 	 */
 	void invalidate() {
-		boolean isIndexTable = tableRecord.getIndexedColumn() >= 0;
 		tableRecord = null;
 		rootBufferId = -1;
 		nodeMgr = null;
 		++modCount;
-		if (!isIndexTable) {
-			db.tableDeleted(this);
-		}
 	}
 
 	/**

Ghidra/Framework/DB/src/test/java/db/DBTest.java

@@ -19,7 +19,7 @@ import static org.junit.Assert.*;
 
 import java.io.File;
 import java.io.IOException;
-import java.util.Arrays;
+import java.util.*;
 
 import org.junit.*;
 
@@ -46,12 +46,15 @@ public class DBTest extends AbstractGenericTest {
 	private BufferFileManager fileMgr;
 	private DBHandle dbh;
 	private BufferFile bfile;
+	private MyDbListener listener;
 
 	@Before
 	public void setUp() throws Exception {
 
 		testDir = createTempDirectory(getClass().getSimpleName());
 		dbh = new DBHandle(BUFFER_SIZE, CACHE_SIZE);
+		listener = new MyDbListener();
+		dbh.addListener(listener);
 	}
 
 	@After
@@ -548,4 +551,108 @@ public class DBTest extends AbstractGenericTest {
 		assertNotNull(t1prime.getRecord(1));
 
 	}
+
+	@Test
+	public void testEvents() throws IOException {
+
+		createIndexedTables(false);
+
+		// Verify table added events
+		Table[] tables = dbh.getTables();
+		assertEquals(11, tables.length);
+		assertEquals(tables.length, listener.events.size());
+		int ix = 0;
+		for (DbEvent evt : listener.events) {
+			assertEquals(DbNotifyType.TABLE_ADDED, evt.type);
+			assertEquals("TABLE" + ix, evt.table.getName());
+			++ix;
+		}
+
+		listener.events.clear();
+
+		// Verify table deleted event
+		long txId = dbh.startTransaction();
+		Table t = dbh.getTable("TABLE5");
+		dbh.deleteTable("TABLE5");
+		dbh.endTransaction(txId, true);
+		assertEquals(1, listener.events.size());
+		DbEvent evt = listener.events.get(0);
+		assertEquals(DbNotifyType.TABLE_DELETED, evt.type);
+		assertTrue(t == evt.table);
+
+		listener.events.clear();
+
+		dbh.undo();
+
+		assertEquals(1, listener.events.size());
+		evt = listener.events.get(0);
+		assertEquals(DbNotifyType.RESTORED, evt.type);
+
+		listener.events.clear();
+
+		dbh.redo();
+
+		assertEquals(1, listener.events.size());
+		evt = listener.events.get(0);
+		assertEquals(DbNotifyType.RESTORED, evt.type);
+
+		listener.events.clear();
+
+		dbh.close();
+		dbh = null;
+
+		assertEquals(1, listener.events.size());
+		evt = listener.events.get(0);
+		assertEquals(DbNotifyType.CLOSED, evt.type);
+
+	}
+
+	private enum DbNotifyType {
+		RESTORED, CLOSED, TABLE_DELETED, TABLE_ADDED;
+	}
+
+	private static class DbEvent {
+
+		final DbNotifyType type;
+		final Table table;
+
+		DbEvent(DbNotifyType type, Table table) {
+			this.type = type;
+			this.table = table;
+		}
+
+		@Override
+		public String toString() {
+			if (type != DbNotifyType.TABLE_ADDED) {
+				return type.toString();
+			}
+			return type + ": " + table.getName();
+		}
+	}
+
+	private static class MyDbListener implements DBListener {
+
+		private List<DbEvent> events = new ArrayList<>();
+
+		@Override
+		public void dbRestored(DBHandle dbh) {
+			events.add(new DbEvent(DbNotifyType.RESTORED, null));
+		}
+
+		@Override
+		public void dbClosed(DBHandle dbh) {
+			events.add(new DbEvent(DbNotifyType.CLOSED, null));
+		}
+
+		@Override
+		public void tableDeleted(DBHandle dbh, Table table) {
+			events.add(new DbEvent(DbNotifyType.TABLE_DELETED, table));
+		}
+
+		@Override
+		public void tableAdded(DBHandle dbh, Table table) {
+			events.add(new DbEvent(DbNotifyType.TABLE_ADDED, table));
+		}
+
+	}
 }

Ghidra/Framework/Project/src/main/java/ghidra/framework/data/AbstractTransactionManager.java

@@ -101,6 +101,13 @@ abstract class AbstractTransactionManager {
 		return null;
 	}
 
+	/**
+	 * Force transaction lock and terminate current transaction.
+	 * @param rollback true if rollback of non-commited changes should occurs, false if commit
+	 * should be done.  NOTE: it can be potentially detrimental to commit an incomplete transaction
+	 * and should be avoided.
+	 * @param reason very short reason for requesting lock
+	 */
 	final void forceLock(boolean rollback, String reason) {
 
 		synchronized (this) {
@@ -119,6 +126,13 @@ abstract class AbstractTransactionManager {
 		terminateTransaction(rollback, true);
 	}
 
+	/**
+	 * Terminate current transaction.
+	 * @param rollback true if rollback of non-commited changes should occurs, false if commit
+	 * should be done.  NOTE: it can be potentially detrimental to commit an incomplete transaction
+	 * and should be avoided.
+	 * @param notify true for listeners to be notified else false
+	 */
 	abstract void terminateTransaction(boolean rollback, boolean notify);
 
 	final synchronized void unlock() {

Ghidra/Framework/Project/src/main/java/ghidra/framework/model/DomainObject.java

@@ -284,8 +284,10 @@ public interface DomainObject {
 	public boolean lock(String reason);
 
 	/**
-	 * Cancels any previous lock and acquires it.
-	 * @param rollback if true, any changes in made with the previous lock should be discarded.
+	 * Force transaction lock and terminate current transaction.
+	 * @param rollback true if rollback of non-commited changes should occurs, false if commit
+	 * should be done.  NOTE: it can be potentially detrimental to commit an incomplete transaction
+	 * which should be avoided.
 	 * @param reason very short reason for requesting lock
 	 */
 	public void forceLock(boolean rollback, String reason);