[GitHub] [lucene-solr] muse-dev[bot] commented on a change in pull request #1962: SOLR-14749 Provide a clean API for cluster-level event processing

[GitBox <gi...@apache.org>]

muse-dev[bot] commented on a change in pull request #1962:
URL: https://github.com/apache/lucene-solr/pull/1962#discussion_r516960732



##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ClusterEventProducerBase.java
##########
@@ -0,0 +1,85 @@
+package org.apache.solr.cluster.events;
+
+import org.apache.solr.core.CoreContainer;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.lang.invoke.MethodHandles;
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
+/**
+ *
+ */
+public abstract class ClusterEventProducerBase implements ClusterEventProducer {
+  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+  protected final Map<ClusterEvent.EventType, Set<ClusterEventListener>> listeners = new ConcurrentHashMap<>();
+  protected volatile State state = State.STOPPED;
+  protected final CoreContainer cc;
+
+  protected ClusterEventProducerBase(CoreContainer cc) {
+    this.cc = cc;
+  }
+
+  @Override
+  public void registerListener(ClusterEventListener listener, ClusterEvent.EventType... eventTypes) {
+    if (eventTypes == null || eventTypes.length == 0) {
+      eventTypes = ClusterEvent.EventType.values();
+    }
+    for (ClusterEvent.EventType type : eventTypes) {
+      if (!getSupportedEventTypes().contains(type)) {
+        log.warn("event type {} not supported yet.", type);
+        continue;
+      }
+      // to avoid removing no-longer empty set in unregister
+      synchronized (listeners) {
+        listeners.computeIfAbsent(type, t -> ConcurrentHashMap.newKeySet())
+            .add(listener);
+      }
+    }
+  }
+
+  @Override
+  public void unregisterListener(ClusterEventListener listener, ClusterEvent.EventType... eventTypes) {
+    if (eventTypes == null || eventTypes.length == 0) {
+      eventTypes = ClusterEvent.EventType.values();
+    }
+    synchronized (listeners) {
+      for (ClusterEvent.EventType type : eventTypes) {
+        Set<ClusterEventListener> perType = listeners.get(type);
+        if (perType != null) {
+          perType.remove(listener);
+          if (perType.isEmpty()) {
+            listeners.remove(type);
+          }
+        }
+      }
+    }
+  }
+
+  @Override
+  public State getState() {
+    return state;
+  }
+
+  public Map<ClusterEvent.EventType, Set<ClusterEventListener>> getEventListeners() {
+    return listeners;
+  }
+
+  public CoreContainer getCoreContainer() {
+    return cc;
+  }
+
+  public abstract Set<ClusterEvent.EventType> getSupportedEventTypes();
+
+  protected void fireEvent(ClusterEvent event) {
+    listeners.getOrDefault(event.getType(), Collections.emptySet())

Review comment:
       *THREAD_SAFETY_VIOLATION:*  Read/Write race. Non-private method `ClusterEventProducerBase.fireEvent(...)` reads without synchronization from container `this.listeners` via call to `Map.getOrDefault(...)`. Potentially races with write in method `ClusterEventProducerBase.unregisterListener(...)`.
    Reporting because another access to the same memory occurs on a background thread, although this access may not.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org