Merge pull request #311 from tevans/OF-921

OF-921: Reset MUC group cache for group changes

src/java/org/jivesoftware/openfire/group/ConcurrentGroupList.java

@@ -16,13 +16,14 @@ import org.xmpp.packet.JID;
  */
 public class ConcurrentGroupList<T> extends CopyOnWriteArrayList<T> implements GroupAwareList<T> {
 
-	private static final long serialVersionUID = -8884698048047935327L;
-	
+	private static final long serialVersionUID = 7735849143650412115L;
+
 	// This set is used to optimize group operations within this list.
 	// We only populate this set when it's needed to dereference the
 	// groups in the base list, but once it exists we keep it in sync
 	// via the various add/remove operations.
-	private transient Set<Group> groupsInList;
+	// NOTE: added volatile keyword for double-check idiom (lazy instantiation)
+	private volatile transient Set<String> knownGroupNamesInList;
 	
 	public ConcurrentGroupList() {
 		super();
@@ -61,20 +62,41 @@ public class ConcurrentGroupList<T> extends CopyOnWriteArrayList<T> implements G
 	 * @return A Set containing the groups in the list
 	 */
 	@Override
-	public synchronized Set<Group> getGroups() {
-		if (groupsInList == null) {
-			groupsInList = new HashSet<Group>();
-			// add all the groups into the group set
-			Iterator<T> iterator = iterator();
-			while (iterator.hasNext()) {
-				T listItem = iterator.next();
-				Group group = Group.resolveFrom(listItem);
-				if (group != null) {
-					groupsInList.add(group);
-				};
+	public Set<Group> getGroups() {
+		Set<Group> result = new HashSet<Group>();
+		for (String groupName : getKnownGroupNamesInList()) {
+			result.add(Group.resolveFrom(groupName));
+		}
+		return result;
+	}
+	
+	/**
+	 * Accessor uses the  "double-check idiom" (j2se 5.0+) for proper lazy instantiation.
+	 * Additionally, the set is not cached until there is at least one group in the list.
+	 * 
+	 * @return the known group names among the items in the list
+	 */
+	private Set<String> getKnownGroupNamesInList() {
+		Set<String> result = knownGroupNamesInList;
+		if (result == null) {
+			synchronized(this) {
+				result = knownGroupNamesInList;
+				if (result == null) {
+					result = new HashSet<String>();
+					// add all the groups into the group set
+					Iterator<T> iterator = iterator();
+					while (iterator.hasNext()) {
+						T listItem = iterator.next();
+						Group group = Group.resolveFrom(listItem);
+						if (group != null) {
+							result.add(group.getName());
+						};
+					}
+					knownGroupNamesInList = result.isEmpty() ? null : result;
+				}
 			}
 		}
-		return groupsInList;
+		return result;
 	}
 
 	/**
@@ -88,14 +110,17 @@ public class ConcurrentGroupList<T> extends CopyOnWriteArrayList<T> implements G
 	private synchronized boolean syncGroups(Object item, boolean addOrRemove) {
 		boolean result = false;
 		// only sync if the group list has been instantiated
-		if (groupsInList != null) {
+		if (knownGroupNamesInList != null) {
 			Group group = Group.resolveFrom(item);
 			if (group != null) {
 				result = true;
 				if (addOrRemove == ADD) {
-					groupsInList.add(group);
+					knownGroupNamesInList.add(group.getName());
 				} else if (addOrRemove == REMOVE) {
-					groupsInList.remove(group);
+					knownGroupNamesInList.remove(group.getName());
+					if (knownGroupNamesInList.isEmpty()) {
+						knownGroupNamesInList = null;
+					}
 				}
 			}
 		}
@@ -155,7 +180,7 @@ public class ConcurrentGroupList<T> extends CopyOnWriteArrayList<T> implements G
 		if (changed) {
 			// drop the transient set, will be rebuilt when/if needed
 			synchronized(this) {
-				groupsInList = null;
+				knownGroupNamesInList = null;
 			}
 		}
 		return changed;
@@ -167,7 +192,7 @@ public class ConcurrentGroupList<T> extends CopyOnWriteArrayList<T> implements G
 		if (changed) {
 			// drop the transient set, will be rebuilt when/if needed
 			synchronized(this) {
-				groupsInList = null;
+				knownGroupNamesInList = null;
 			}
 		}
 		return changed;
@@ -179,7 +204,7 @@ public class ConcurrentGroupList<T> extends CopyOnWriteArrayList<T> implements G
 		if (added > 0) {
 			// drop the transient set, will be rebuilt when/if needed
 			synchronized(this) {
-				groupsInList = null;
+				knownGroupNamesInList = null;
 			}
 		}
 		return added;
@@ -189,7 +214,7 @@ public class ConcurrentGroupList<T> extends CopyOnWriteArrayList<T> implements G
 	public void clear() {
 		super.clear();
 		synchronized(this) {
-			groupsInList = null;
+			knownGroupNamesInList = null;
 		}
 	}
 
@@ -199,7 +224,7 @@ public class ConcurrentGroupList<T> extends CopyOnWriteArrayList<T> implements G
 		if (changed) {
 			// drop the transient set, will be rebuilt when/if needed
 			synchronized(this) {
-				groupsInList = null;
+				knownGroupNamesInList = null;
 			}
 		}
 		return changed;
@@ -211,7 +236,7 @@ public class ConcurrentGroupList<T> extends CopyOnWriteArrayList<T> implements G
 		if (changed) {
 			// drop the transient set, will be rebuilt when/if needed
 			synchronized(this) {
-				groupsInList = null;
+				knownGroupNamesInList = null;
 			}
 		}
 		return changed;

src/java/org/jivesoftware/openfire/group/ConcurrentGroupMap.java

@@ -17,20 +17,20 @@ import org.xmpp.packet.JID;
 
 public class ConcurrentGroupMap<K, V> extends ConcurrentHashMap<K, V>  implements GroupAwareMap<K, V> {
 
-	private static final long serialVersionUID = -2068242013524715293L;
+	private static final long serialVersionUID = 5479781418678223200L;
 
 	// These sets are used to optimize group operations within this map.
 	// We only populate these sets when they are needed to dereference the
 	// groups in the base map, but once they exist we keep them in sync
 	// via the various put/remove operations.
-	private transient Set<Group> groupsFromKeys;
-	private transient Set<Group> groupsFromValues;
-	
+	// NOTE: added volatile keyword for double-check idiom (lazy instantiation)
+	private volatile transient Set<String> knownGroupNamesFromKeys;
+	private volatile transient Set<String> knownGroupNamesFromValues;
 
 	/**
-	 * Returns true if the key list contains the given JID. If the JID
-	 * is not found in the key list, search the key list for groups and 
-	 * look for the JID in each of the corresponding groups.
+	 * Returns true if the key list contains the given JID. If the JID is not found in the 
+	 * key list (exact match), search the key list for groups and look for the JID in 
+	 * each of the corresponding groups (implied match).
 	 * 
 	 * @param key The target, presumably a JID
 	 * @return True if the target is in the key list, or in any groups in the key list
@@ -52,9 +52,9 @@ public class ConcurrentGroupMap<K, V> extends ConcurrentHashMap<K, V>  implement
 
 
 	/**
-	 * Returns true if the map has an entry value matching the given JID. If the JID
-	 * is not found explicitly, search the values for groups and search 
-	 * for the JID in each of the corresponding groups.
+	 * Returns true if the map has an entry value matching the given JID. If the JID is not 
+	 * found in the value set (exact match), search the value set for groups and look for the 
+	 * JID in each of the corresponding groups (implied match).
 	 * 
 	 * @param value The target, presumably a JID
 	 * @return True if the target is in the value set, or in any groups in the value set
@@ -64,7 +64,7 @@ public class ConcurrentGroupMap<K, V> extends ConcurrentHashMap<K, V>  implement
 		if (containsValue(value)) {
 			found = true;
 		} else if (value instanceof JID) {
-			// look for group JIDs in the list of keys and dereference as needed
+			// look for group JIDs in the list of values and dereference as needed
 			JID target = (JID) value;
 			Iterator<Group> iterator = getGroupsFromValues().iterator();
 			while (!found && iterator.hasNext()) {
@@ -81,19 +81,11 @@ public class ConcurrentGroupMap<K, V> extends ConcurrentHashMap<K, V>  implement
 	 */
 	@Override
 	public synchronized Set<Group> getGroupsFromKeys() {
-		if (groupsFromKeys == null) {
-			groupsFromKeys = new HashSet<Group>();
-			// add all the groups into the group set
-			Iterator<K> iterator = keySet().iterator();
-			while (iterator.hasNext()) {
-				K key = iterator.next();
-				Group group = Group.resolveFrom(key);
-				if (group != null) {
-					groupsFromKeys.add(group);
-				};
-			}
+		Set<Group> result = new HashSet<Group>();
+		for(String groupName : getKnownGroupNamesFromKeys()) {
+			result.add(Group.resolveFrom(groupName));
 		}
-		return groupsFromKeys;
+		return result;
 	}
 
 	/**
@@ -103,19 +95,71 @@ public class ConcurrentGroupMap<K, V> extends ConcurrentHashMap<K, V>  implement
 	 */
 	@Override
 	public synchronized Set<Group> getGroupsFromValues() {
-		if (groupsFromValues == null) {
-			groupsFromValues = new HashSet<Group>();
-			// add all the groups into the group set
-			Iterator<V> iterator = values().iterator();
-			while (iterator.hasNext()) {
-				V value = iterator.next();
-				Group group = Group.resolveFrom(value);
-				if (group != null) {
-					groupsFromValues.add(group);
-				};
+		Set<Group> result = new HashSet<Group>();
+		for(String groupName : getKnownGroupNamesFromValues()) {
+			result.add(Group.resolveFrom(groupName));
+		}
+		return result;
+	}
+
+	
+	/**
+	 * Accessor uses the  "double-check idiom" (j2se 5.0+) for proper lazy instantiation.
+	 * Additionally, nothing is cached until there is at least one group in the map's keys.
+	 * 
+	 * @return the known group names among the items in the list
+	 */
+	private Set<String> getKnownGroupNamesFromKeys() {
+		Set<String> result = knownGroupNamesFromKeys;
+		if (result == null) {
+			synchronized(this) {
+				result = knownGroupNamesFromKeys;
+				if (result == null) {
+					result = new HashSet<String>();
+					// add all the groups into the group set
+					Iterator<K> iterator = keySet().iterator();
+					while (iterator.hasNext()) {
+						K key = iterator.next();
+						Group group = Group.resolveFrom(key);
+						if (group != null) {
+							result.add(group.getName());
+						};
+					}
+					knownGroupNamesFromKeys = result.isEmpty() ? null : result;
+				}
 			}
 		}
-		return groupsFromValues;
+		return result;
+	}
+
+	
+	/**
+	 * Accessor uses the  "double-check idiom" (j2se 5.0+) for proper lazy instantiation.
+	 * Additionally, nothing is cached until there is at least one group in the map's value set.
+	 * 
+	 * @return the known group names among the items in the list
+	 */
+	private Set<String> getKnownGroupNamesFromValues() {
+		Set<String> result = knownGroupNamesFromValues;
+		if (result == null) {
+			synchronized(this) {
+				result = knownGroupNamesFromValues;
+				if (result == null) {
+					result = new HashSet<String>();
+					// add all the groups into the group set
+					Iterator<V> iterator = values().iterator();
+					while (iterator.hasNext()) {
+						V key = iterator.next();
+						Group group = Group.resolveFrom(key);
+						if (group != null) {
+							result.add(group.getName());
+						};
+					}
+					knownGroupNamesFromValues = result.isEmpty() ? null : result;
+				}
+			}
+		}
+		return result;
 	}
 
 	/**
@@ -129,16 +173,23 @@ public class ConcurrentGroupMap<K, V> extends ConcurrentHashMap<K, V>  implement
 	 */
 	private synchronized boolean syncGroups(Object item, boolean keyOrValue, boolean addOrRemove) {
 		boolean result = false;
-		Set<Group> groupSet = (keyOrValue == KEYS) ? groupsFromKeys : groupsFromValues;
+		Set<String> groupSet = (keyOrValue == KEYS) ? knownGroupNamesFromKeys : knownGroupNamesFromValues;
 		// only sync if the group list has been instantiated
 		if (groupSet != null) {
 			Group group = Group.resolveFrom(item);
 			if (group != null) {
 				result = true;
 				if (addOrRemove == ADD) {
-					groupSet.add(group);
+					groupSet.add(group.getName());
 				} else if (addOrRemove == REMOVE) {
-					groupSet.remove(group);
+					groupSet.remove(group.getName());
+					if (groupSet.isEmpty()) {
+						if (keyOrValue == KEYS) {
+							knownGroupNamesFromKeys = null;
+						} else {
+							knownGroupNamesFromValues = null;
+						}
+					}
 				}
 			}
 		}
@@ -180,10 +231,7 @@ public class ConcurrentGroupMap<K, V> extends ConcurrentHashMap<K, V>  implement
 	public void putAll(Map<? extends K, ? extends V> m) {
 		super.putAll(m);
 		// drop the transient sets; will be rebuilt when/if needed
-		synchronized(this) {
-			groupsFromKeys = null;
-			groupsFromValues = null;
-		}
+		clearCache();
 	}
 
 
@@ -234,14 +282,24 @@ public class ConcurrentGroupMap<K, V> extends ConcurrentHashMap<K, V>  implement
 	@Override
 	public void clear() {
 		super.clear();
-		synchronized(this) {
-			groupsFromKeys = null;
-			groupsFromValues = null;
-		}
+		clearCache();
+	}
+	
+	/**
+	 * Certain operations imply that our locally cached group list(s) should
+	 * be dropped and recreated. For example, when an ad-hoc client command
+	 * is used to add a member to a group, the underlying group instance is
+	 * actually dropped from the global Group cache, with the effect that our
+	 * cache would be referring to an orphaned Group instance.
+	 */
+	private synchronized void clearCache() {
+		knownGroupNamesFromKeys = null;
+		knownGroupNamesFromValues = null;
 	}
 
 	private static final boolean KEYS = true;
 	private static final boolean VALUES = false;
 	private static final boolean ADD = true;
 	private static final boolean REMOVE = false;
+
 }

src/java/org/jivesoftware/openfire/group/GroupAwareMap.java

@@ -23,7 +23,7 @@ public interface GroupAwareMap<K, V> extends Map<K, V> {
 	public boolean includesKey(Object key);
 
 	/**
-	 * Returns true if the map has a key referencing the given JID. If the JID
+	 * Returns true if the map contains a value referencing the given JID. If the JID
 	 * is not found explicitly, search the values for groups and look 
 	 * for the JID in each of the corresponding groups.
 	 *