Merge remote-tracking branch 'origin/GP-361-dragonmacher-copy-on-read-fixes--SQUASHED'

Ghidra/Framework/Generic/src/main/java/ghidra/util/datastruct/CopyOnReadWeakSet.java

@@ -16,53 +16,43 @@
 package ghidra.util.datastruct;
 
 import java.util.*;
+import java.util.stream.Stream;
 
-public class CopyOnReadWeakSet<T> extends WeakSet<T> {
+/**
+ * A copy on read set that will create a copy of its internal data for iteration operations.  This
+ * allows clients to avoid concurrency issue by allowing mutates during reads.  All operations
+ * of this class are synchronized to allow clients to use non-iterative methods without the need
+ * for a copy operation.
+ *
+ * @param <T> the type
+ */
+class CopyOnReadWeakSet<T> extends WeakSet<T> {
 
 	protected CopyOnReadWeakSet() {
 		// restrict access; use factory method in WeakDataStructureFactory
 	}
 
-	/**
+	private synchronized Collection<T> createCopy() {
-	 * Add the given object to the set.
+		Set<T> ks = weakHashStorage.keySet();
-	 */
+		return new ArrayList<>(ks);
+	}
+
 	@Override
 	public synchronized void add(T t) {
 		maybeWarnAboutAnonymousValue(t);
 		weakHashStorage.put(t, null);
 	}
 
-	/**
-	 * Remove the given object from the data structure
-	 */
 	@Override
 	public synchronized void remove(T t) {
 		weakHashStorage.remove(t);
 	}
 
-	/**
-	 * Remove all elements from this data structure
-	 */
 	@Override
 	public synchronized void clear() {
 		weakHashStorage.clear();
 	}
 
-	/**
-	 * Returns an iterator over the elements in this data structure.
-	 */
-	@Override
-	public synchronized Iterator<T> iterator() {
-		Set<T> ks = weakHashStorage.keySet();
-		List<T> list = new ArrayList<>(ks);
-		return list.iterator();
-	}
-
-	@Override
-	public synchronized Collection<T> values() {
-		return weakHashStorage.keySet();
-	}
-
 	@Override
 	public synchronized boolean isEmpty() {
 		return weakHashStorage.isEmpty();
@@ -77,4 +67,25 @@ public class CopyOnReadWeakSet<T> extends WeakSet<T> {
 	public synchronized boolean contains(T t) {
 		return weakHashStorage.containsKey(t);
 	}
+
+	@Override
+	public synchronized String toString() {
+		return weakHashStorage.keySet().toString();
+	}
+
+	@Override
+	public synchronized Iterator<T> iterator() {
+		return createCopy().iterator();
+	}
+
+	@Override
+	public synchronized Collection<T> values() {
+		return createCopy();
+	}
+
+	@Override
+	public synchronized Stream<T> stream() {
+		return createCopy().stream();
+	}
+
 }

Ghidra/Framework/Generic/src/main/java/ghidra/util/datastruct/CopyOnWriteWeakSet.java

@@ -16,6 +16,7 @@
 package ghidra.util.datastruct;
 
 import java.util.*;
+import java.util.stream.Stream;
 
 import org.apache.commons.collections4.IteratorUtils;
 
@@ -25,12 +26,12 @@ import org.apache.commons.collections4.IteratorUtils;
  * number of event notification operations significantly out numbers mutations to this structure
  * (e.g., adding and removing items.
  * <p>
- * An example use cases where using this class is a good fit would be a listener list where
+ * An example use case 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 is used to fire a large number of events.
  * <p>
  * 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.
+ * contents are changed often, but iterated very little.
  * <p>
  * 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
@@ -47,7 +48,7 @@ class CopyOnWriteWeakSet<T> extends WeakSet<T> {
 	}
 
 	@Override
-	public synchronized Iterator<T> iterator() {
+	public Iterator<T> iterator() {
 		return IteratorUtils.unmodifiableIterator(weakHashStorage.keySet().iterator());
 	}
 
@@ -60,26 +61,29 @@ class CopyOnWriteWeakSet<T> extends WeakSet<T> {
 	 * @param it the items
 	 */
 	@Override
-	public void addAll(Iterable<T> it) {
+	public synchronized void addAll(Iterable<T> it) {
 		// only make one copy for the entire set of changes instead of for each change, as calling
 		// add() would do
-		weakHashStorage = new WeakHashMap<>(weakHashStorage);
+		WeakHashMap<T, T> newStorage = new WeakHashMap<>(weakHashStorage);
 		for (T t : it) {
-			weakHashStorage.put(t, null);
+			newStorage.put(t, null);
 		}
+		weakHashStorage = newStorage;
 	}
 
 	@Override
 	public synchronized void add(T t) {
 		maybeWarnAboutAnonymousValue(t);
-		weakHashStorage = new WeakHashMap<>(weakHashStorage);
+		WeakHashMap<T, T> newStorage = new WeakHashMap<>(weakHashStorage);
-		weakHashStorage.put(t, null);
+		newStorage.put(t, null);
+		weakHashStorage = newStorage;
 	}
 
 	@Override
 	public synchronized void remove(T t) {
-		weakHashStorage = new WeakHashMap<>(weakHashStorage);
+		WeakHashMap<T, T> newStorage = new WeakHashMap<>(weakHashStorage);
-		weakHashStorage.remove(t);
+		newStorage.remove(t);
+		weakHashStorage = newStorage;
 	}
 
 	@Override
@@ -88,22 +92,32 @@ class CopyOnWriteWeakSet<T> extends WeakSet<T> {
 	}
 
 	@Override
-	public synchronized Collection<T> values() {
+	public Collection<T> values() {
 		return weakHashStorage.keySet();
 	}
 
 	@Override
-	public synchronized boolean isEmpty() {
+	public boolean isEmpty() {
 		return weakHashStorage.isEmpty();
 	}
 
 	@Override
-	public synchronized int size() {
+	public int size() {
 		return weakHashStorage.size();
 	}
 
 	@Override
-	public synchronized boolean contains(T t) {
+	public boolean contains(T t) {
 		return weakHashStorage.containsKey(t);
 	}
+
+	@Override
+	public Stream<T> stream() {
+		return values().stream();
+	}
+
+	@Override
+	public String toString() {
+		return values().toString();
+	}
 }

Ghidra/Framework/Generic/src/main/java/ghidra/util/datastruct/ThreadUnsafeWeakSet.java

@@ -17,6 +17,7 @@ package ghidra.util.datastruct;
 
 import java.util.Collection;
 import java.util.Iterator;
+import java.util.stream.Stream;
 
 class ThreadUnsafeWeakSet<T> extends WeakSet<T> {
 
@@ -24,34 +25,22 @@ class ThreadUnsafeWeakSet<T> extends WeakSet<T> {
 		// restrict access; use factory method in base class
 	}
 
-	/**
-	 * Add the given object to the set.
-	 */
 	@Override
 	public void add(T t) {
 		maybeWarnAboutAnonymousValue(t);
 		weakHashStorage.put(t, null);
 	}
 
-	/**
-	 * Remove the given object from the data structure
-	 */
 	@Override
 	public void remove(T t) {
 		weakHashStorage.remove(t);
 	}
 
-	/**
-	 * Remove all elements from this data structure
-	 */
 	@Override
 	public void clear() {
 		weakHashStorage.clear();
 	}
 
-	/**
-	 * Returns an iterator over the elements in this data structure.
-	 */
 	@Override
 	public Iterator<T> iterator() {
 		return weakHashStorage.keySet().iterator();
@@ -81,4 +70,10 @@ class ThreadUnsafeWeakSet<T> extends WeakSet<T> {
 	public String toString() {
 		return weakHashStorage.toString();
 	}
+
+	@Override
+	public Stream<T> stream() {
+		return values().stream();
+	}
+
 }

Ghidra/Framework/Generic/src/main/java/ghidra/util/datastruct/WeakSet.java

@@ -136,12 +136,5 @@ public abstract class WeakSet<T> implements Iterable<T> {
 	 * Returns a stream of the values of this collection.
 	 * @return a stream of the values of this collection.
 	 */
-	public Stream<T> stream() {
+	public abstract Stream<T> stream();
-		return values().stream();
-	}
-
-	@Override
-	public String toString() {
-		return values().toString();
-	}
 }