diff --git a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/tracemgr/DebuggerTraceManagerServicePlugin.java b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/tracemgr/DebuggerTraceManagerServicePlugin.java index 7bd047d237..0825d1328d 100644 --- a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/tracemgr/DebuggerTraceManagerServicePlugin.java +++ b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/tracemgr/DebuggerTraceManagerServicePlugin.java @@ -634,7 +634,7 @@ public class DebuggerTraceManagerServicePlugin extends Plugin } @Override - public synchronized Collection getOpenTraces() { + public Collection getOpenTraces() { synchronized (listenersByTrace) { return Set.copyOf(tracesView); } diff --git a/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/DBTrace.java b/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/DBTrace.java index 36021a0204..1c9c6284af 100644 --- a/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/DBTrace.java +++ b/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/DBTrace.java @@ -15,7 +15,6 @@ */ package ghidra.trace.database; -import java.io.File; import java.io.IOException; import java.util.*; import java.util.function.Consumer; @@ -876,16 +875,10 @@ public class DBTrace extends DBCachedDomainObjectAdapter implements Trace, Trace } @Override - public void save(String comment, TaskMonitor monitor) throws IOException, CancelledException { - objectManager.flushWbCaches(); - super.save(comment, monitor); - } - - @Override - public void saveToPackedFile(File outputFile, TaskMonitor monitor) - throws IOException, CancelledException { - objectManager.flushWbCaches(); - super.saveToPackedFile(outputFile, monitor); + protected void prepareToSave() { + try (Transaction tx = openForcedTransaction("flush for save")) { + objectManager.flushWbCaches(); + } } public boolean isClosing() { @@ -895,9 +888,9 @@ public class DBTrace extends DBCachedDomainObjectAdapter implements Trace, Trace @Override protected void close() { closing = true; - objectManager.flushWbCaches(); - super.close(); + // NOTE: Any unsaved changes in the write-back cache are unrecoverable objectManager.waitWbWorkers(); + super.close(); } @Override diff --git a/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/DBTraceContentHandler.java b/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/DBTraceContentHandler.java index 093b571e13..a562af81e0 100644 --- a/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/DBTraceContentHandler.java +++ b/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/DBTraceContentHandler.java @@ -41,16 +41,16 @@ public class DBTraceContentHandler extends DBWithUserDataContentHandler static final Class TRACE_DOMAIN_OBJECT_CLASS = DBTrace.class; static final String TRACE_CONTENT_DEFAULT_TOOL = "Debugger"; - private static final DBTraceLinkContentHandler linkHandler = new DBTraceLinkContentHandler(); + private static final DBTraceLinkContentHandler LINK_HANDLER = new DBTraceLinkContentHandler(); @Override public long createFile(FileSystem fs, FileSystem userfs, String path, String name, DomainObject obj, TaskMonitor monitor) throws IOException, InvalidNameException, CancelledException { - if (!(obj instanceof DBTrace)) { + if (!(obj instanceof DBTrace trace)) { throw new IOException("Unsupported domain object: " + obj.getClass().getName()); } - return createFile((DBTrace) obj, TRACE_CONTENT_TYPE, fs, path, name, monitor); + return createFile(trace, TRACE_CONTENT_TYPE, fs, path, name, monitor); } @Override @@ -340,6 +340,6 @@ public class DBTraceContentHandler extends DBWithUserDataContentHandler @Override public DBTraceLinkContentHandler getLinkHandler() { - return linkHandler; + return LINK_HANDLER; } } diff --git a/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/target/DBTraceObjectValueWriteBehindCache.java b/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/target/DBTraceObjectValueWriteBehindCache.java index 13b0481e16..896a3d2b0d 100644 --- a/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/target/DBTraceObjectValueWriteBehindCache.java +++ b/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/target/DBTraceObjectValueWriteBehindCache.java @@ -40,8 +40,9 @@ class DBTraceObjectValueWriteBehindCache { private final AsyncReference busy = new AsyncReference<>(false); private volatile boolean flushing = false; - private final Map>> cachedValues = - new HashMap<>(); + private final Map>> cachedValues = + new HashMap<>(); public DBTraceObjectValueWriteBehindCache(DBTraceObjectManager manager) { this.manager = manager; @@ -51,7 +52,7 @@ class DBTraceObjectValueWriteBehindCache { } private void workLoop() { - while (!manager.trace.isClosed()) { + while (!manager.trace.isClosing()) { try { synchronized (cachedValues) { if (cachedValues.isEmpty()) { @@ -69,7 +70,7 @@ class DBTraceObjectValueWriteBehindCache { cachedValues.wait(left); } } - if (manager.trace.isClosed()) { + if (manager.trace.isClosing()) { break; } writeBatch(); diff --git a/Ghidra/Features/Base/src/test.slow/java/ghidra/framework/data/TransactionLockingTest.java b/Ghidra/Features/Base/src/test.slow/java/ghidra/framework/data/TransactionLockingTest.java index b751a7e0a7..ea826a49ed 100644 --- a/Ghidra/Features/Base/src/test.slow/java/ghidra/framework/data/TransactionLockingTest.java +++ b/Ghidra/Features/Base/src/test.slow/java/ghidra/framework/data/TransactionLockingTest.java @@ -4,9 +4,9 @@ * 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. diff --git a/Ghidra/Framework/Project/src/main/java/ghidra/framework/data/AbstractTransactionManager.java b/Ghidra/Framework/Project/src/main/java/ghidra/framework/data/AbstractTransactionManager.java index 4d794d3e16..4f4b9efc94 100644 --- a/Ghidra/Framework/Project/src/main/java/ghidra/framework/data/AbstractTransactionManager.java +++ b/Ghidra/Framework/Project/src/main/java/ghidra/framework/data/AbstractTransactionManager.java @@ -4,9 +4,9 @@ * 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. @@ -55,31 +55,36 @@ abstract class AbstractTransactionManager { checkLockingTask(); + boolean doPrepare = false; synchronized (this) { if (getCurrentTransactionInfo() != null && !transactionTerminated) { return false; } if (lockCount == 0) { - for (DomainObjectAdapterDB domainObj : getDomainObjects()) { - if (domainObj.isChanged()) { - domainObj.prepareToSave(); - } - } + doPrepare = true; } lockReason = reason; ++lockCount; - return true; } + if (doPrepare) { + for (DomainObjectAdapterDB domainObj : getDomainObjects()) { + if (domainObj.isChanged()) { + domainObj.prepareToSave(); + } + } + } + return true; } /** - * Attempt to obtain a modification lock on the domain object when generating a - * background snapshot. + * Attempt to obtain a modification lock on the domain object when generating a background + * snapshot. + * * @param domainObj domain object corresponding to snapshot * @param hasProgress true if monitor has progress indicator * @param title title to be used for monitor * @return monitor object if lock obtained successfully, else null which indicates that a - * modification is in process. + * modification is in process. */ final synchronized LockingTaskMonitor lockForSnapshot(DomainObjectAdapterDB domainObj, boolean hasProgress, String title) { @@ -106,9 +111,10 @@ abstract class AbstractTransactionManager { /** * 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. + * 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) { @@ -131,9 +137,10 @@ abstract class AbstractTransactionManager { /** * 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. + * 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); @@ -159,8 +166,7 @@ abstract class AbstractTransactionManager { } /** - * Block on active locking task. - * Do not invoke this method from within a synchronized block. + * Block on active locking task. Do not invoke this method from within a synchronized block. */ final void checkLockingTask() { synchronized (this) { @@ -173,6 +179,7 @@ abstract class AbstractTransactionManager { /** * Throw lock exception if currently locked + * * @throws DomainObjectLockedException if currently locked */ final void verifyNoLock() throws DomainObjectLockedException { @@ -194,7 +201,7 @@ abstract class AbstractTransactionManager { } } - final int startTransaction(DomainObjectAdapterDB object, String description, + final int startTransactionChecked(DomainObjectAdapterDB object, String description, AbortedTransactionListener listener, boolean notify) throws TerminatedTransactionException { @@ -222,47 +229,53 @@ abstract class AbstractTransactionManager { boolean commit, boolean notify) throws IllegalStateException; /** - * Returns the undo stack depth. - * (The number of items on the undo stack) - * This method is for JUnits. + * Returns the undo stack depth. (The number of items on the undo stack) This method is for + * JUnits. + * * @return the undo stack depth */ abstract int getUndoStackDepth(); /** * Returns true if there is at least one redo transaction to be redone. + * * @return true if there is at least one redo transaction to be redone */ abstract boolean canRedo(); /** * Returns true if there is at least one undo transaction to be undone. + * * @return true if there is at least one undo transaction to be undone */ abstract boolean canUndo(); /** * Returns the name of the next undo transaction (The most recent change). + * * @return the name of the next undo transaction (The most recent change) */ abstract String getRedoName(); /** * Returns the name of the next redo transaction (The most recent undo). + * * @return the name of the next redo transaction (The most recent undo) */ abstract String getUndoName(); /** - * Returns the names of all undoable transactions in reverse chronological order. In other - * words the transaction at the top of the list must be undone first. + * Returns the names of all undoable transactions in reverse chronological order. In other words + * the transaction at the top of the list must be undone first. + * * @return the names of all undoable transactions in reverse chronological order */ abstract List getAllUndoNames(); /** - * Returns the names of all redoable transactions in chronological order. In other words - * the transaction at the top of the list must be redone first. + * Returns the names of all redoable transactions in chronological order. In other words the + * transaction at the top of the list must be redone first. + * * @return the names of all redoable transactions in chronological order */ abstract List getAllRedoNames(); @@ -321,7 +334,7 @@ abstract class AbstractTransactionManager { abstract void doClose(DomainObjectAdapterDB object); /** - * Set instance as immutable by disabling use of transactions. Attempts to start a transaction + * Set instance as immutable by disabling use of transactions. Attempts to start a transaction * will result in a {@link TerminatedTransactionException}. */ public void setImmutable() { diff --git a/Ghidra/Framework/Project/src/main/java/ghidra/framework/data/DomainObjectAdapterDB.java b/Ghidra/Framework/Project/src/main/java/ghidra/framework/data/DomainObjectAdapterDB.java index 6a76c0d7c2..0b87dc05c1 100644 --- a/Ghidra/Framework/Project/src/main/java/ghidra/framework/data/DomainObjectAdapterDB.java +++ b/Ghidra/Framework/Project/src/main/java/ghidra/framework/data/DomainObjectAdapterDB.java @@ -133,7 +133,7 @@ public abstract class DomainObjectAdapterDB extends DomainObjectAdapter implemen * using a shared transaction manager. If either or both is already shared, * a transition to a single shared transaction manager will be * performed. - * @param domainObj + * @param domainObj the domain object to synchronize with * @throws LockException if lock or open transaction is active on either * this or the specified domain object */ @@ -184,7 +184,7 @@ public abstract class DomainObjectAdapterDB extends DomainObjectAdapter implemen } /** - * Returns the open handle to the underlying database. + * {@return the open handle to the underlying database} */ public DBHandle getDBHandle() { return dbh; @@ -259,17 +259,17 @@ public abstract class DomainObjectAdapterDB extends DomainObjectAdapter implemen return transactionMgr.lock(reason); } - void prepareToSave() { - int txId = transactionMgr.startTransaction(this, "Update Metadata", null, true, true); - try { + /** + * Prepare to save and store any last minute DB data. The default behavior is to invoke + * {@link #updateMetadata()}. + */ + protected void prepareToSave() { + try (Transaction tx = openForcedTransaction("Update Metadata")) { updateMetadata(); } catch (IOException e) { dbError(e); } - finally { - transactionMgr.endTransaction(this, txId, true, true); - } } /** @@ -310,24 +310,40 @@ public abstract class DomainObjectAdapterDB extends DomainObjectAdapter implemen transactionMgr.unlock(handler); } + private class DomainObjectTransaction extends Transaction { + private final int txId; + + DomainObjectTransaction(int txId) { + this.txId = txId; + } + + @Override + protected boolean endTransaction(boolean commit) { + DomainObjectAdapterDB.this.endTransaction(txId, commit); + return commit; + } + + @Override + public boolean isSubTransaction() { + return true; + } + } + + /** + * Open forced transaction which bypasses any lock checking, although method should only be + * used in very controlled situations where a save lock is in place. + * @param description transaction description + * @return {@link AutoCloseable} transaction object + */ + protected Transaction openForcedTransaction(String description) { + int txId = transactionMgr.startTransaction(this, description, null, true, true); + return new DomainObjectTransaction(txId); + } + @Override public Transaction openTransaction(String description) throws TerminatedTransactionException, IllegalStateException { - return new Transaction() { - - int txId = startTransaction(description); - - @Override - protected boolean endTransaction(boolean commit) { - DomainObjectAdapterDB.this.endTransaction(txId, commit); - return commit; - } - - @Override - public boolean isSubTransaction() { - return true; - } - }; + return new DomainObjectTransaction(startTransaction(description)); } @Override @@ -348,7 +364,8 @@ public abstract class DomainObjectAdapterDB extends DomainObjectAdapter implemen while (true) { try { - return transactionMgr.startTransaction(this, description, listener, true); + return transactionMgr.startTransactionChecked(this, description, listener, + true); } catch (DomainObjectLockedException e) { if (!exceptionHandler.apply(e)) { diff --git a/Ghidra/Framework/Project/src/main/java/ghidra/framework/data/DomainObjectTransactionManager.java b/Ghidra/Framework/Project/src/main/java/ghidra/framework/data/DomainObjectTransactionManager.java index b4d98f16e7..d967888480 100644 --- a/Ghidra/Framework/Project/src/main/java/ghidra/framework/data/DomainObjectTransactionManager.java +++ b/Ghidra/Framework/Project/src/main/java/ghidra/framework/data/DomainObjectTransactionManager.java @@ -103,11 +103,12 @@ class DomainObjectTransactionManager extends AbstractTransactionManager { throw new IllegalArgumentException("invalid domain object"); } - if (!force) { - verifyNoLock(); - } - if (transaction == null) { + + if (!force) { + verifyNoLock(); + } + transactionTerminated = false; transaction = new DomainObjectDBTransaction(domainObj.dbh.startTransaction(), domainObj); diff --git a/Ghidra/Framework/Project/src/main/java/ghidra/framework/data/GhidraFolderData.java b/Ghidra/Framework/Project/src/main/java/ghidra/framework/data/GhidraFolderData.java index d7685ee830..6295aaf801 100644 --- a/Ghidra/Framework/Project/src/main/java/ghidra/framework/data/GhidraFolderData.java +++ b/Ghidra/Framework/Project/src/main/java/ghidra/framework/data/GhidraFolderData.java @@ -1037,7 +1037,7 @@ class GhidraFolderData { if (doa.isClosed()) { throw new ClosedException(); } - if (!doa.lock(null)) { + if (!doa.lock("create file")) { throw new IOException("Object is busy and can not be saved"); } diff --git a/Ghidra/Framework/Project/src/main/java/ghidra/framework/data/SynchronizedTransactionManager.java b/Ghidra/Framework/Project/src/main/java/ghidra/framework/data/SynchronizedTransactionManager.java index 5665413f7c..5afa66dd38 100644 --- a/Ghidra/Framework/Project/src/main/java/ghidra/framework/data/SynchronizedTransactionManager.java +++ b/Ghidra/Framework/Project/src/main/java/ghidra/framework/data/SynchronizedTransactionManager.java @@ -4,9 +4,9 @@ * 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. @@ -152,11 +152,12 @@ class SynchronizedTransactionManager extends AbstractTransactionManager { synchronized int startTransaction(DomainObjectAdapterDB object, String description, AbortedTransactionListener listener, boolean force, boolean notify) { - if (!force) { - verifyNoLock(); - } - if (transaction == null) { + + if (!force) { + verifyNoLock(); + } + transactionTerminated = false; transaction = new SynchronizedTransaction(domainObjectTransactionManagers); int txId = transaction.addEntry(object, description, listener); diff --git a/Ghidra/Framework/Project/src/main/java/ghidra/framework/main/FileActionManager.java b/Ghidra/Framework/Project/src/main/java/ghidra/framework/main/FileActionManager.java index 4d24d55083..c98df4ac00 100644 --- a/Ghidra/Framework/Project/src/main/java/ghidra/framework/main/FileActionManager.java +++ b/Ghidra/Framework/Project/src/main/java/ghidra/framework/main/FileActionManager.java @@ -311,7 +311,7 @@ class FileActionManager { locked = false; break; } - if (!domainObjects[lastIndex].lock(null)) { + if (!domainObjects[lastIndex].lock("save changes")) { String title = "Exit Ghidra"; StringBuffer buf = new StringBuffer(); DomainObject d = domainObjects[lastIndex]; diff --git a/Ghidra/Framework/Project/src/main/java/ghidra/framework/model/DomainObject.java b/Ghidra/Framework/Project/src/main/java/ghidra/framework/model/DomainObject.java index fd0820e88f..9255d26daa 100644 --- a/Ghidra/Framework/Project/src/main/java/ghidra/framework/model/DomainObject.java +++ b/Ghidra/Framework/Project/src/main/java/ghidra/framework/model/DomainObject.java @@ -400,7 +400,7 @@ public interface DomainObject { * * * @param description a short description of the changes to be made. - * @return transaction object + * @return {@link AutoCloseable} transaction object * @throws IllegalStateException if this {@link DomainObject} has already been closed. */ public Transaction openTransaction(String description) throws IllegalStateException;