Fix notification listeners leak (MID-5355)

Due to a fault in ChangeNotificationDispatcherImpl change listeners were not unregistered correctly. This might have a lot of various consequences, including memory leaks, performance degradation, and ConcurrentModificationException's on cache content (e.g. MID-5355). This commit also introduces checks for (future) listener leaks. (cherry picked from commit 05e83a7)
Evolveum · May 23, 2019 · c07382a · c07382a
1 parent 39708fa
commit c07382a
Show file tree

Hide file tree

Showing 3 changed files with 21 additions and 7 deletions.
diff --git a/...int/model/common/expression/evaluator/caching/AbstractSearchExpressionEvaluatorCache.java b/...int/model/common/expression/evaluator/caching/AbstractSearchExpressionEvaluatorCache.java
@@ -27,9 +27,9 @@
 import com.evolveum.midpoint.xml.ns._public.common.common_3.ObjectSearchStrategyType;
 import com.evolveum.midpoint.xml.ns._public.common.common_3.ObjectType;
 
-import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
 
 /**
  * Cache for search expression-based evaluators.
@@ -63,7 +63,9 @@ public void setClientContextInformation(Object clientContextInformation) {
         this.clientContextInformation = clientContextInformation;
     }
 
-    protected Map<QK, QR> queries = new HashMap<>();
+    // ConcurrentHashMap should not be required here because the cache should be accessed only from a single thread.
+    // However, there could be bugs that make this assumption invalid (see e.g. MID-5355). So just for sure let's make this thread-safe.
+    protected Map<QK, QR> queries = new ConcurrentHashMap<>();
 
     public List<V> getQueryResult(Class<? extends ObjectType> type, ObjectQuery query, ObjectSearchStrategyType searchStrategy, ExpressionEvaluationContext params, PrismContext prismContext) {
         QK queryKey = createQueryKey(type, query, searchStrategy, params, prismContext);

diff --git a/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/lens/Clockwork.java b/model/model-impl/src/main/java/com/evolveum/midpoint/model/impl/lens/Clockwork.java
@@ -433,11 +433,13 @@ private void enterAssociationSearchExpressionEvaluatorCache() {
 	private void exitAssociationSearchExpressionEvaluatorCache() {
 		AssociationSearchExpressionEvaluatorCache cache = AssociationSearchExpressionEvaluatorCache.exitCache();
 		if (cache == null) {
-			return;			// shouldn't occur
+			LOGGER.error("exitAssociationSearchExpressionEvaluatorCache: cache instance was not found for the current thread");
+			return;
 		}
 		Object invalidator = cache.getClientContextInformation();
 		if (!(invalidator instanceof AssociationSearchExpressionCacheInvalidator)) {
-			return;			// shouldn't occur either
+			LOGGER.error("exitAssociationSearchExpressionEvaluatorCache: expected {}, got {} instead", AssociationSearchExpressionCacheInvalidator.class, invalidator);
+			return;
 		}
 		changeNotificationDispatcher.unregisterNotificationListener((ResourceObjectChangeListener) invalidator);
 		changeNotificationDispatcher.unregisterNotificationListener((ResourceOperationListener) invalidator);

diff --git a/...c/main/java/com/evolveum/midpoint/provisioning/impl/ChangeNotificationDispatcherImpl.java b/...c/main/java/com/evolveum/midpoint/provisioning/impl/ChangeNotificationDispatcherImpl.java
@@ -29,7 +29,6 @@
 import com.evolveum.midpoint.provisioning.api.ResourceObjectShadowChangeDescription;
 import com.evolveum.midpoint.provisioning.api.ResourceOperationDescription;
 import com.evolveum.midpoint.provisioning.api.ResourceOperationListener;
-import com.evolveum.midpoint.repo.api.PreconditionViolationException;
 import com.evolveum.midpoint.schema.internals.InternalsConfig;
 import com.evolveum.midpoint.schema.result.OperationResult;
 import com.evolveum.midpoint.task.api.Task;
@@ -51,6 +50,8 @@
 @Component
 public class ChangeNotificationDispatcherImpl implements ChangeNotificationDispatcher {
 
+	private static final int MAX_LISTENERS = 1000;          // just to make sure we don't grow indefinitely
+
 	private boolean filterProtectedObjects = true;
 	private List<ResourceObjectChangeListener> changeListeners = new ArrayList<>();     // use synchronized access only!
 	private List<ResourceOperationListener> operationListeners = new ArrayList<>();
@@ -79,6 +80,13 @@ public synchronized void registerNotificationListener(ResourceObjectChangeListen
 					listener);
 		} else {
 			changeListeners.add(listener);
+			checkSize(changeListeners, "changeListeners");
+		}
+	}
+
+	private void checkSize(List<?> listeners, String name) {
+		if (listeners.size() > MAX_LISTENERS) {
+			throw new IllegalStateException("Possible listeners leak: number of " + name + " exceeded the threshold of " + MAX_LISTENERS);
 		}
 	}
 
@@ -103,6 +111,7 @@ public synchronized void registerNotificationListener(ResourceOperationListener
 					listener);
 		} else {
 			operationListeners.add(listener);
+			checkSize(operationListeners, "operationListeners");
 		}
 
 	}
@@ -116,6 +125,7 @@ public synchronized void registerNotificationListener(ResourceEventListener list
 					listener);
 		} else {
 			eventListeners.add(listener);
+			checkSize(eventListeners, "eventListeners");
 		}
 
 	}
@@ -128,12 +138,12 @@ public void unregisterNotificationListener(ResourceEventListener listener) {
 
 	@Override
 	public synchronized void unregisterNotificationListener(ResourceOperationListener listener) {
-		changeListeners.remove(listener);
+		operationListeners.remove(listener);
 	}
 
 	@Override
 	public synchronized void unregisterNotificationListener(ResourceObjectChangeListener listener) {
-		operationListeners.remove(listener);
+		changeListeners.remove(listener);
 	}
 
 	@Override