From 1ef3f71dd165ccde8e7e4bf2e2aba8ed48c06bc7 Mon Sep 17 00:00:00 2001 From: dragonmacher <48328597+dragonmacher@users.noreply.github.com> Date: Tue, 29 Mar 2022 15:30:47 -0400 Subject: [PATCH] GP-1861 - Changed locking to prevent potential out-of-order events --- .../data/DomainObjectEventQueues.java | 18 +- .../trace/database/DBTraceUserData.java | 4 +- .../database/program/DBTraceProgramView.java | 2 +- .../program/DBTraceProgramViewRegisters.java | 2 +- .../framework/data/DBDomainObjectSupport.java | 2 +- .../ghidra/feature/vt/api/db/VTSessionDB.java | 2 +- .../util/datastruct/CopyOnWriteWeakSet.java | 14 +- .../framework/data/DomainObjectAdapter.java | 77 ++---- .../framework/data/DomainObjectAdapterDB.java | 6 +- .../data/DomainObjectChangeSupport.java | 244 ++++++++++-------- .../ghidra/framework/data/GhidraFileData.java | 2 +- .../framework/data/DummyDomainObject.java | 2 +- .../framework/task/GenericDomainObjectDB.java | 2 +- .../program/database/DataTypeArchiveDB.java | 4 +- .../ghidra/program/database/ProgramDB.java | 4 +- .../program/database/ProgramUserDataDB.java | 4 +- 16 files changed, 192 insertions(+), 197 deletions(-) diff --git a/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/framework/data/DomainObjectEventQueues.java b/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/framework/data/DomainObjectEventQueues.java index 0f08e2ae4b..a064c4b6bb 100644 --- a/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/framework/data/DomainObjectEventQueues.java +++ b/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/framework/data/DomainObjectEventQueues.java @@ -28,15 +28,19 @@ public class DomainObjectEventQueues { protected final DomainObject source; protected final Lock lock; protected final DomainObjectChangeSupport eventQueue; - protected final Map privateEventQueues = CacheBuilder - .newBuilder().removalListener(this::privateQueueRemoved).weakKeys().build().asMap(); + protected final Map privateEventQueues = + CacheBuilder.newBuilder() + .removalListener(this::privateQueueRemoved) + .weakKeys() + .build() + .asMap(); protected volatile boolean eventsEnabled = true; - public DomainObjectEventQueues(DomainObject source, int timeInterval, int bufsize, Lock lock) { + public DomainObjectEventQueues(DomainObject source, int timeInterval, Lock lock) { this.source = source; this.lock = lock; - eventQueue = new DomainObjectChangeSupport(source, timeInterval, bufsize, lock); + eventQueue = new DomainObjectChangeSupport(source, timeInterval, lock); } private void privateQueueRemoved( @@ -51,18 +55,18 @@ public class DomainObjectEventQueues { } } - public synchronized void addListener(DomainObjectListener l) { + public void addListener(DomainObjectListener l) { eventQueue.addListener(l); } - public synchronized void removeListener(DomainObjectListener l) { + public void removeListener(DomainObjectListener l) { eventQueue.removeListener(l); } public EventQueueID createPrivateEventQueue(DomainObjectListener listener, int maxDelay) { EventQueueID id = new EventQueueID(); DomainObjectChangeSupport privateQueue = - new DomainObjectChangeSupport(source, maxDelay, 1000, lock); + new DomainObjectChangeSupport(source, maxDelay, lock); privateQueue.addListener(listener); privateEventQueues.put(id, privateQueue); return id; diff --git a/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/DBTraceUserData.java b/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/DBTraceUserData.java index fa4782470b..a3460efff7 100644 --- a/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/DBTraceUserData.java +++ b/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/DBTraceUserData.java @@ -29,12 +29,12 @@ public class DBTraceUserData extends DomainObjectAdapterDB implements TraceUserD } protected DBTraceUserData(DBTrace trace) throws IOException { - super(new DBHandle(), getName(trace), 500, 1000, trace); + super(new DBHandle(), getName(trace), 500, trace); // TODO: Create database and such } public DBTraceUserData(DBHandle dbh, DBTrace trace, TaskMonitor monitor) { - super(dbh, getName(trace), 500, 1000, trace); + super(dbh, getName(trace), 500, trace); // TODO Auto-generated constructor stub } diff --git a/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/program/DBTraceProgramView.java b/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/program/DBTraceProgramView.java index 98475ec6ae..47a517649d 100644 --- a/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/program/DBTraceProgramView.java +++ b/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/program/DBTraceProgramView.java @@ -913,7 +913,7 @@ public class DBTraceProgramView implements TraceProgramView { this.viewport.setSnap(snap); this.eventQueues = - new DomainObjectEventQueues(this, TIME_INTERVAL, BUF_SIZE, trace.getLock()); + new DomainObjectEventQueues(this, TIME_INTERVAL, trace.getLock()); this.regViewsByThread = new WeakValueHashMap<>(); diff --git a/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/program/DBTraceProgramViewRegisters.java b/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/program/DBTraceProgramViewRegisters.java index 1109ed1f40..0856f1801e 100644 --- a/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/program/DBTraceProgramViewRegisters.java +++ b/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/program/DBTraceProgramViewRegisters.java @@ -61,7 +61,7 @@ public class DBTraceProgramViewRegisters implements TraceProgramView { this.thread = codeSpace.getThread(); // TODO: Bleh, should be parameter this.eventQueues = new DomainObjectEventQueues(this, DBTraceProgramView.TIME_INTERVAL, - DBTraceProgramView.BUF_SIZE, view.trace.getLock()); + view.trace.getLock()); // TODO: Make these create code/memory spaces lazily, to allow null at construction // NOTE: Use reference manager as example diff --git a/Ghidra/Debug/ProposedUtils/src/main/java/ghidra/framework/data/DBDomainObjectSupport.java b/Ghidra/Debug/ProposedUtils/src/main/java/ghidra/framework/data/DBDomainObjectSupport.java index 899af3167f..0288f349af 100644 --- a/Ghidra/Debug/ProposedUtils/src/main/java/ghidra/framework/data/DBDomainObjectSupport.java +++ b/Ghidra/Debug/ProposedUtils/src/main/java/ghidra/framework/data/DBDomainObjectSupport.java @@ -38,7 +38,7 @@ public abstract class DBDomainObjectSupport extends DomainObjectAdapterDB { protected DBDomainObjectSupport(DBHandle dbh, DBOpenMode openMode, TaskMonitor monitor, String name, int timeInterval, int bufSize, Object consumer) { - super(dbh, name, timeInterval, bufSize, consumer); + super(dbh, name, timeInterval, consumer); this.openMode = openMode; this.monitor = monitor; } diff --git a/Ghidra/Features/VersionTracking/src/main/java/ghidra/feature/vt/api/db/VTSessionDB.java b/Ghidra/Features/VersionTracking/src/main/java/ghidra/feature/vt/api/db/VTSessionDB.java index 55545d5cdc..8c1278cb7b 100644 --- a/Ghidra/Features/VersionTracking/src/main/java/ghidra/feature/vt/api/db/VTSessionDB.java +++ b/Ghidra/Features/VersionTracking/src/main/java/ghidra/feature/vt/api/db/VTSessionDB.java @@ -211,7 +211,7 @@ public class VTSessionDB extends DomainObjectAdapterDB implements VTSession, VTC } private VTSessionDB(DBHandle dbHandle, Object consumer) { - super(dbHandle, UNUSED_DEFAULT_NAME, EVENT_NOTIFICATION_DELAY, EVENT_BUFFER_SIZE, consumer); + super(dbHandle, UNUSED_DEFAULT_NAME, EVENT_NOTIFICATION_DELAY, consumer); propertyTable = dbHandle.getTable(PROPERTY_TABLE_NAME); } diff --git a/Ghidra/Framework/Generic/src/main/java/ghidra/util/datastruct/CopyOnWriteWeakSet.java b/Ghidra/Framework/Generic/src/main/java/ghidra/util/datastruct/CopyOnWriteWeakSet.java index 0e9d83face..9a16242917 100644 --- a/Ghidra/Framework/Generic/src/main/java/ghidra/util/datastruct/CopyOnWriteWeakSet.java +++ b/Ghidra/Framework/Generic/src/main/java/ghidra/util/datastruct/CopyOnWriteWeakSet.java @@ -20,24 +20,24 @@ import java.util.*; import org.apache.commons.collections4.IteratorUtils; /** - * A set that avoids {@link ConcurrentModificationException}s by copying the internal storage - * for every mutation operation. Thus, this data structure is only efficient when the + * A set that avoids {@link ConcurrentModificationException}s by copying the internal storage + * for every mutation operation. Thus, this data structure is only efficient when the * number of event notification operations significantly out numbers mutations to this structure * (e.g., adding and removing items. *

- * An example use cases where using this class is a good fit would be a listener list where - * listeners are added during initialization, but not after that. Further, this hypothetical + * An example use cases where using this class is a good fit would be a listener list where + * listeners are added during initialization, but not after that. Further, this hypothetical * list fires a large number of events. *

- * A bad use of this class would be as a container to store widgets where the container the + * A bad use of this class would be as a container to store widgets where the container the * contents are changed often, but iterated over very little. *

* Finally, if this structure is only ever used from a single thread, like the Swing thread, then * you do not need the overhead of this class, as the Swing thread synchronous access guarantees - * that the structure cannot be mutated while it is being iterated. See + * that the structure cannot be mutated while it is being iterated. See * {@link WeakDataStructureFactory#createSingleThreadAccessWeakSet()}. * - * @param + * @param the type * @see WeakSet */ class CopyOnWriteWeakSet extends WeakSet { diff --git a/Ghidra/Framework/Project/src/main/java/ghidra/framework/data/DomainObjectAdapter.java b/Ghidra/Framework/Project/src/main/java/ghidra/framework/data/DomainObjectAdapter.java index 386d68396d..8e039a42e5 100644 --- a/Ghidra/Framework/Project/src/main/java/ghidra/framework/data/DomainObjectAdapter.java +++ b/Ghidra/Framework/Project/src/main/java/ghidra/framework/data/DomainObjectAdapter.java @@ -60,9 +60,8 @@ public abstract class DomainObjectAdapter implements DomainObject { private ArrayList consumers; protected Map metadata = new LinkedHashMap(); - // a flag indicating whether the domain object has changed - // any methods of this domain object which cause its state to - // to change must set this flag to true + // A flag indicating whether the domain object has changed. Any methods of this domain object + // which cause its state to change must set this flag to true protected boolean changed = false; // a flag indicating that this object is temporary @@ -73,19 +72,18 @@ public abstract class DomainObjectAdapter implements DomainObject { /** * Construct a new DomainObjectAdapter. If construction of this object fails, be sure to release * with consumer. - * + * * @param name name of the object * @param timeInterval the time (in milliseconds) to wait before the event queue is flushed. If * a new event comes in before the time expires, the timer is reset. - * @param bufsize initial size of event buffer * @param consumer the object that created this domain object */ - protected DomainObjectAdapter(String name, int timeInterval, int bufsize, Object consumer) { + protected DomainObjectAdapter(String name, int timeInterval, Object consumer) { if (consumer == null) { throw new IllegalArgumentException("Consumer must not be null"); } this.name = name; - docs = new DomainObjectChangeSupport(this, timeInterval, bufsize, lock); + docs = new DomainObjectChangeSupport(this, timeInterval, lock); consumers = new ArrayList(); consumers.add(consumer); if (!UserData.class.isAssignableFrom(getClass())) { @@ -94,9 +92,6 @@ public abstract class DomainObjectAdapter implements DomainObject { } } - /** - * @see ghidra.framework.model.DomainObject#release(java.lang.Object) - */ @Override public void release(Object consumer) { synchronized (consumers) { @@ -115,9 +110,6 @@ public abstract class DomainObjectAdapter implements DomainObject { return lock; } - /** - * @see ghidra.framework.model.DomainObject#getDomainFile() - */ @Override public DomainFile getDomainFile() { return domainFile; @@ -126,7 +118,7 @@ public abstract class DomainObjectAdapter implements DomainObject { /** * Returns the hidden user-filesystem associated with this objects domain file, or null if * unknown. - * + * * @return user data file system */ protected FileSystem getAssociatedUserFilesystem() { @@ -136,17 +128,11 @@ public abstract class DomainObjectAdapter implements DomainObject { return null; } - /** - * @see ghidra.framework.model.DomainObject#getName() - */ @Override public String getName() { return name; } - /** - * @see java.lang.Object#toString() - */ @Override public String toString() { String classname = getClass().getName(); @@ -154,9 +140,6 @@ public abstract class DomainObjectAdapter implements DomainObject { return name + " - " + classname; } - /** - * @see ghidra.framework.model.DomainObject#setName(java.lang.String) - */ @Override public void setName(String newName) { synchronized (this) { @@ -180,25 +163,16 @@ public abstract class DomainObjectAdapter implements DomainObject { } } - /** - * @see ghidra.framework.model.DomainObject#isChanged() - */ @Override public boolean isChanged() { return changed && !temporary; } - /** - * @see ghidra.framework.model.DomainObject#setTemporary(boolean) - */ @Override public void setTemporary(boolean state) { temporary = state; } - /** - * @see ghidra.framework.model.DomainObject#isTemporary() - */ @Override public boolean isTemporary() { return temporary; @@ -237,9 +211,6 @@ public abstract class DomainObjectAdapter implements DomainObject { closeListeners.clear(); } - /** - * @see ghidra.framework.model.DomainObject#flushEvents() - */ @Override public void flushEvents() { docs.flush(); @@ -250,24 +221,18 @@ public abstract class DomainObjectAdapter implements DomainObject { /** * Return "changed" status - * + * * @return true if this object has changed */ public boolean getChangeStatus() { return changed; } - /** - * @see ghidra.framework.model.DomainObject#addListener(ghidra.framework.model.DomainObjectListener) - */ @Override public void addListener(DomainObjectListener l) { docs.addListener(l); } - /** - * @see ghidra.framework.model.DomainObject#removeListener(ghidra.framework.model.DomainObjectListener) - */ @Override public void removeListener(DomainObjectListener l) { docs.removeListener(l); @@ -286,7 +251,7 @@ public abstract class DomainObjectAdapter implements DomainObject { @Override public EventQueueID createPrivateEventQueue(DomainObjectListener listener, int maxDelay) { EventQueueID eventQueueID = new EventQueueID(); - DomainObjectChangeSupport queue = new DomainObjectChangeSupport(this, maxDelay, 1000, lock); + DomainObjectChangeSupport queue = new DomainObjectChangeSupport(this, maxDelay, lock); queue.addListener(listener); changeSupportMap.put(eventQueueID, queue); return eventQueueID; @@ -310,15 +275,12 @@ public abstract class DomainObjectAdapter implements DomainObject { } } - /** - * @see ghidra.framework.model.DomainObject#getDescription() - */ @Override public abstract String getDescription(); /** * Fires the specified event. - * + * * @param ev event to fire */ public void fireEvent(DomainObjectChangeRecord ev) { @@ -331,9 +293,6 @@ public abstract class DomainObjectAdapter implements DomainObject { } } - /** - * @see ghidra.framework.model.DomainObject#setEventsEnabled(boolean) - */ @Override public void setEventsEnabled(boolean v) { if (eventsEnabled != v) { @@ -353,9 +312,6 @@ public abstract class DomainObjectAdapter implements DomainObject { return eventsEnabled; } - /** - * @see ghidra.framework.model.DomainObject#hasExclusiveAccess() - */ @Override public boolean hasExclusiveAccess() { return domainFile == null || !domainFile.isCheckedOut() || @@ -372,9 +328,6 @@ public abstract class DomainObjectAdapter implements DomainObject { changed = state; } - /** - * @see ghidra.framework.model.DomainObject#addConsumer(java.lang.Object) - */ @Override public boolean addConsumer(Object consumer) { if (consumer == null) { @@ -403,7 +356,9 @@ public abstract class DomainObjectAdapter implements DomainObject { } /** - * Returns true if the this file is used only by the given tool + * Returns true if the this file is used only by the given consumer + * @param consumer the consumer + * @return true if the this file is used only by the given consumer */ boolean isUsedExclusivelyBy(Object consumer) { synchronized (consumers) { @@ -430,7 +385,7 @@ public abstract class DomainObjectAdapter implements DomainObject { /** * Set default content type - * + * * @param doClass default domain object implementation */ public static synchronized void setDefaultContentClass(Class doClass) { @@ -450,9 +405,10 @@ public abstract class DomainObjectAdapter implements DomainObject { /** * Get the ContentHandler associated with the specified content-type. - * + * * @param contentType domain object content type * @return content handler + * @throws IOException if no content handler can be found */ static synchronized ContentHandler getContentHandler(String contentType) throws IOException { checkContentHandlerMaps(); @@ -465,9 +421,10 @@ public abstract class DomainObjectAdapter implements DomainObject { /** * Get the ContentHandler associated with the specified domain object - * + * * @param dobj domain object * @return content handler + * @throws IOException if no content handler can be found */ public static synchronized ContentHandler getContentHandler(DomainObject dobj) throws IOException { 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 9b28d45eea..35e48094b4 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 @@ -90,12 +90,10 @@ public abstract class DomainObjectAdapterDB extends DomainObjectAdapter * @param timeInterval the time (in milliseconds) to wait before the * event queue is flushed. If a new event comes in before the time expires, * the timer is reset. - * @param bufSize initial size of event buffer * @param consumer the object that created this domain object */ - protected DomainObjectAdapterDB(DBHandle dbh, String name, int timeInterval, int bufSize, - Object consumer) { - super(name, timeInterval, bufSize, consumer); + protected DomainObjectAdapterDB(DBHandle dbh, String name, int timeInterval, Object consumer) { + super(name, timeInterval, consumer); this.dbh = dbh; options = new OptionsDB(this); transactionMgr = new DomainObjectTransactionManager(this); diff --git a/Ghidra/Framework/Project/src/main/java/ghidra/framework/data/DomainObjectChangeSupport.java b/Ghidra/Framework/Project/src/main/java/ghidra/framework/data/DomainObjectChangeSupport.java index ff34748ded..9b1a07cef2 100644 --- a/Ghidra/Framework/Project/src/main/java/ghidra/framework/data/DomainObjectChangeSupport.java +++ b/Ghidra/Framework/Project/src/main/java/ghidra/framework/data/DomainObjectChangeSupport.java @@ -25,13 +25,24 @@ import ghidra.util.*; import ghidra.util.datastruct.WeakDataStructureFactory; import ghidra.util.datastruct.WeakSet; +/** + * A class to queue and send {@link DomainObjectChangeRecord} events. + *

+ * For simplicity, this class requires all mutations to internal data structures to be locked using + * the internal write lock. Clients are not required to use any synchronization when using this + * class. + *

+ * Internally, events are queued and will be fired on a timer. + */ class DomainObjectChangeSupport { - private WeakSet listeners; - private DomainObject src; - private List changesQueue; + private WeakSet listeners = + WeakDataStructureFactory.createSingleThreadAccessWeakSet(); + private List notificationQueue = new ArrayList<>(); + private List recordsQueue = new ArrayList<>(); private GhidraTimer timer; + private DomainObject src; private Lock domainObjectLock; private Lock writeLock = new Lock("DOCS Change Records Queue Lock"); @@ -39,114 +50,95 @@ class DomainObjectChangeSupport { /** * Constructs a new DomainObjectChangeSupport object. - * + * * @param src The object to be put as the src for all events generated. * @param timeInterval The time (in milliseconds) this object will wait before flushing its - * event buffer. If a new event comes in before the time expires, the timer is reset. + * event buffer. If a new event comes in before the time expires, the timer is reset. * @param lock the lock used to verify that calls to {@link #flush()} are not performed while a - * lock is held; this is the lock to guard the DB + * lock is held; this is the lock to guard the DB */ - DomainObjectChangeSupport(DomainObject src, int timeInterval, int bufsize, Lock lock) { + DomainObjectChangeSupport(DomainObject src, int timeInterval, Lock lock) { - this.src = src; + this.src = Objects.requireNonNull(src); this.domainObjectLock = Objects.requireNonNull(lock); - changesQueue = new ArrayList<>(bufsize); - - listeners = WeakDataStructureFactory.createCopyOnWriteWeakSet(); - - timer = GhidraTimerFactory.getGhidraTimer(timeInterval, timeInterval, () -> sendEventNow()); + this.timer = + GhidraTimerFactory.getGhidraTimer(timeInterval, timeInterval, this::sendEventNow); timer.setInitialDelay(25); timer.setDelay(500); timer.setRepeats(true); } - void addListener(DomainObjectListener listener) { - - // Capture the pending event to send to the existing listeners. This prevents the new - // listener from getting events registered before the listener was added. - DomainObjectChangedEvent pendingEvent; - List previousListeners; - synchronized (src) { - pendingEvent = convertEventQueueRecordsToEvent(); - previousListeners = atomicAddListener(listener); - } - - /* - * Do outside the synchronized block, so that we do not get this deadlock: - * Thread1 has Domain Lock -> wants AWT lock - * Swing has AWT lock -> wants Domain lock - */ - SystemUtilities.runIfSwingOrPostSwingLater( - () -> notifyEvent(previousListeners, pendingEvent)); - } - - void removeListener(DomainObjectListener listener) { - synchronized (src) { - listeners.remove(listener); - } - } - + // Note: must be called on the Swing thread private void sendEventNow() { - DomainObjectChangedEvent ev = convertEventQueueRecordsToEvent(); - notifyEvent(listeners, ev); - } + List notifications = withLock(() -> { - private DomainObjectChangedEvent convertEventQueueRecordsToEvent() { - - DomainObjectChangedEvent event = lockQueue(() -> { - - if (changesQueue.isEmpty()) { - timer.stop(); - return null; - } - - DomainObjectChangedEvent e = new DomainObjectChangedEvent(src, changesQueue); - changesQueue = new ArrayList<>(); - return e; + DomainObjectChangedEvent e = createEventFromQueuedRecords(); + notificationQueue.add(new EventNotification(e, new ArrayList<>(listeners.values()))); + List existingNotifications = new ArrayList<>(notificationQueue); + notificationQueue.clear(); + return existingNotifications; }); - return event; + for (EventNotification notification : notifications) { + notification.doNotify(); + } } - // This version of notify takes in the listeners to notify so that we can send events to - // some listeners, but not all of them (like flushing when adding new listeners) - private void notifyEvent(Iterable listenersToNotify, - DomainObjectChangedEvent ev) { + // Note: must be called inside of withLock() + private DomainObjectChangedEvent createEventFromQueuedRecords() { - if (ev == null) { - return; // this implies there we no changes when the timer expired + if (recordsQueue.isEmpty()) { + timer.stop(); + return null; } + DomainObjectChangedEvent e = new DomainObjectChangedEvent(src, recordsQueue); + recordsQueue = new ArrayList<>(); + return e; + } + + void addListener(DomainObjectListener listener) { + if (isDisposed) { return; } - for (DomainObjectListener dol : listenersToNotify) { - try { - dol.domainObjectChanged(ev); - } - catch (Exception exc) { - Msg.showError(this, null, "Error", "Error in Domain Object listener", exc); - } + withLock(() -> { + + // Capture the pending event to send to the existing listeners. This prevents the new + // listener from getting events registered before the listener was added. + DomainObjectChangedEvent pendingEvent = createEventFromQueuedRecords(); + List previousListeners = new ArrayList<>(listeners.values()); + listeners.add(listener); + + notificationQueue.add(new EventNotification(pendingEvent, previousListeners)); + timer.start(); + }); + } + + void removeListener(DomainObjectListener listener) { + if (isDisposed) { + return; } + + withLock(() -> listeners.remove(listener)); } void flush() { Thread lockOwner = domainObjectLock.getOwner(); if (lockOwner == Thread.currentThread()) { - /* - * We have decided that flushing events with a lock can lead to deadlocks. There - * should be no reason to flush events while holding a lock. This is the - * potential deadlock: - * Thread1 has Domain Lock -> wants AWT lock - * Swing has AWT lock -> wants Domain lock - */ - + // + // We have decided that flushing events with a lock can lead to deadlocks. There + // should be no reason to flush events while holding a lock. This is the potential + // deadlock: + // Thread1 has Domain Lock -> wants AWT lock + // Swing has AWT lock -> wants Domain lock + // throw new IllegalStateException("Cannot call flush() with locks!"); } - SystemUtilities.runSwingNow(() -> sendEventNow()); + Swing.runNow(this::sendEventNow); } void fireEvent(DomainObjectChangeRecord docr) { @@ -155,15 +147,20 @@ class DomainObjectChangeSupport { return; } - lockQueue(() -> { - changesQueue.add(docr); + withLock(() -> { + recordsQueue.add(docr); timer.start(); }); } - void fatalErrorOccurred(final Throwable t) { + void fatalErrorOccurred(Throwable t) { - List listenersCopy = new ArrayList<>(listeners.values()); + if (isDisposed) { + return; + } + + List listenersCopy = + withLock(() -> new ArrayList<>(listeners.values())); dispose(); @@ -176,42 +173,35 @@ class DomainObjectChangeSupport { l.domainObjectChanged(ev); } catch (Throwable t2) { - // I guess we don't care (probably because some other fatal error has - // already happened) + // We don't care (probably because some other fatal error has already happened) } } }; - SystemUtilities.runSwingLater(errorTask); + Swing.runLater(errorTask); } void dispose() { - lockQueue(() -> { - isDisposed = true; - timer.stop(); - changesQueue.clear(); - }); - - listeners.clear(); - } - - private List atomicAddListener(DomainObjectListener l) { - - List previousListeners = new ArrayList<>(); - for (DomainObjectListener listener : listeners) { - previousListeners.add(listener); + if (isDisposed) { + return; } - listeners.add(l); - return previousListeners; + withLock(() -> { + isDisposed = true; + timer.stop(); + recordsQueue.clear(); + notificationQueue.clear(); + listeners.clear(); + }); } -//================================================================================================== +//================================================================================================= // Lock Methods -//================================================================================================== +//================================================================================================= - private void lockQueue(Runnable r) { + // Note: all clients of lockQueue() must not call external APIs that could use locking + private void withLock(Runnable r) { try { writeLock.acquire(); @@ -222,7 +212,8 @@ class DomainObjectChangeSupport { } } - private T lockQueue(Callable c) { + // Note: all clients of lockQueue() must not call external APIs that could use locking + private T withLock(Callable c) { try { writeLock.acquire(); @@ -241,4 +232,49 @@ class DomainObjectChangeSupport { writeLock.release(); } } + +//================================================================================================= +// Inner Classes +//================================================================================================= + + /** + * This class allows us to bind the given event with the given listeners. This is used to + * send events to the correct listeners as listeners are added. In other words, new listeners + * will not receive pre-existing buffered events. Also, using this class allows us to ensure + * events are processed linearly by processing each of these notification objects linearly + * from a single queue. + * + * Note: this class shall perform no synchronization; that shall be handled by the client + */ + private class EventNotification { + + private DomainObjectChangedEvent event; + private List receivers; + + EventNotification(DomainObjectChangedEvent event, List recievers) { + this.event = event; + this.receivers = recievers; + } + + // Note: must be called on the Swing thread; must be called outside of lockQueue() + void doNotify() { + + if (isDisposed) { + return; + } + + if (event == null) { + return; // this implies there were no changes when the timer expired + } + + for (DomainObjectListener dol : receivers) { + try { + dol.domainObjectChanged(event); + } + catch (Exception exc) { + Msg.showError(this, null, "Error", "Error in Domain Object listener", exc); + } + } + } + } } diff --git a/Ghidra/Framework/Project/src/main/java/ghidra/framework/data/GhidraFileData.java b/Ghidra/Framework/Project/src/main/java/ghidra/framework/data/GhidraFileData.java index 3d341207ae..17de332757 100644 --- a/Ghidra/Framework/Project/src/main/java/ghidra/framework/data/GhidraFileData.java +++ b/Ghidra/Framework/Project/src/main/java/ghidra/framework/data/GhidraFileData.java @@ -1774,7 +1774,7 @@ public class GhidraFileData { private class GenericDomainObjectDB extends DomainObjectAdapterDB { protected GenericDomainObjectDB(DBHandle dbh) throws IOException { - super(dbh, "Generic", 500, 1000, GhidraFileData.this); + super(dbh, "Generic", 500, GhidraFileData.this); loadMetadata(); } diff --git a/Ghidra/Framework/Project/src/test/java/ghidra/framework/data/DummyDomainObject.java b/Ghidra/Framework/Project/src/test/java/ghidra/framework/data/DummyDomainObject.java index 87cd61175e..3fcf5cc7b3 100644 --- a/Ghidra/Framework/Project/src/test/java/ghidra/framework/data/DummyDomainObject.java +++ b/Ghidra/Framework/Project/src/test/java/ghidra/framework/data/DummyDomainObject.java @@ -29,7 +29,7 @@ public class DummyDomainObject extends DomainObjectAdapterDB { } public DummyDomainObject(String name, Object consumer) throws IOException { - super(new DBHandle(), name, 10, 1, consumer); + super(new DBHandle(), name, 10, consumer); } @Override diff --git a/Ghidra/Framework/Project/src/test/java/ghidra/framework/task/GenericDomainObjectDB.java b/Ghidra/Framework/Project/src/test/java/ghidra/framework/task/GenericDomainObjectDB.java index 449467247c..314a314eab 100644 --- a/Ghidra/Framework/Project/src/test/java/ghidra/framework/task/GenericDomainObjectDB.java +++ b/Ghidra/Framework/Project/src/test/java/ghidra/framework/task/GenericDomainObjectDB.java @@ -28,7 +28,7 @@ public class GenericDomainObjectDB extends DomainObjectAdapterDB { List transactionsList = new ArrayList(); public GenericDomainObjectDB(Object consumer) throws IOException { - super(new DBHandle(), "Generic", 500, 1000, consumer); + super(new DBHandle(), "Generic", 500, consumer); } @Override diff --git a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/DataTypeArchiveDB.java b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/DataTypeArchiveDB.java index 4ebf86be71..3acb67f785 100644 --- a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/DataTypeArchiveDB.java +++ b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/DataTypeArchiveDB.java @@ -100,7 +100,7 @@ public class DataTypeArchiveDB extends DomainObjectAdapterDB */ public DataTypeArchiveDB(DomainFolder folder, String name, Object consumer) throws IOException, DuplicateNameException, InvalidNameException { - super(new DBHandle(), name, 500, 1000, consumer); + super(new DBHandle(), name, 500, consumer); this.name = name; recordChanges = false; @@ -153,7 +153,7 @@ public class DataTypeArchiveDB extends DomainObjectAdapterDB public DataTypeArchiveDB(DBHandle dbh, int openMode, TaskMonitor monitor, Object consumer) throws IOException, VersionException, CancelledException { - super(dbh, "Untitled", 500, 1000, consumer); + super(dbh, "Untitled", 500, consumer); if (monitor == null) { monitor = TaskMonitorAdapter.DUMMY_MONITOR; } diff --git a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/ProgramDB.java b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/ProgramDB.java index d1a9dbf2d5..5ac129ed81 100644 --- a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/ProgramDB.java +++ b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/ProgramDB.java @@ -218,7 +218,7 @@ public class ProgramDB extends DomainObjectAdapterDB implements Program, ChangeM */ public ProgramDB(String name, Language language, CompilerSpec compilerSpec, Object consumer) throws IOException { - super(new DBHandle(), name, 500, 1000, consumer); + super(new DBHandle(), name, 500, consumer); if (!(compilerSpec instanceof BasicCompilerSpec)) { throw new IllegalArgumentException( @@ -287,7 +287,7 @@ public class ProgramDB extends DomainObjectAdapterDB implements Program, ChangeM public ProgramDB(DBHandle dbh, int openMode, TaskMonitor monitor, Object consumer) throws IOException, VersionException, LanguageNotFoundException, CancelledException { - super(dbh, "Untitled", 500, 1000, consumer); + super(dbh, "Untitled", 500, consumer); if (monitor == null) { monitor = TaskMonitor.DUMMY; diff --git a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/ProgramUserDataDB.java b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/ProgramUserDataDB.java index b7d7554cc3..004425c9dd 100644 --- a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/ProgramUserDataDB.java +++ b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/ProgramUserDataDB.java @@ -122,7 +122,7 @@ class ProgramUserDataDB extends DomainObjectAdapterDB implements ProgramUserData } public ProgramUserDataDB(ProgramDB program) throws IOException { - super(new DBHandle(), getName(program), 500, 1000, program); + super(new DBHandle(), getName(program), 500, program); this.program = program; this.language = program.getLanguage(); languageID = language.getLanguageID(); @@ -162,7 +162,7 @@ class ProgramUserDataDB extends DomainObjectAdapterDB implements ProgramUserData public ProgramUserDataDB(DBHandle dbh, ProgramDB program, TaskMonitor monitor) throws IOException, VersionException, LanguageNotFoundException, CancelledException { - super(dbh, getName(program), 500, 1000, program); + super(dbh, getName(program), 500, program); this.program = program; if (monitor == null) { monitor = TaskMonitorAdapter.DUMMY_MONITOR;