SOLR-11255: Use concurrent set for metric names.

solr/CHANGES.txt

@@ -97,6 +97,8 @@ Bug Fixes
 
 * SOLR-11084: Issue with starting script with solr.home (-s) == solr (Leil Ireson, Amrit Sarkar via Erick Erickson)
 
+* SOLR-11255: Fix occasional ConcurrentModificationException when using SolrInfoMBeanHandler. (ab)
+
 Optimizations
 ----------------------
 

solr/core/src/java/org/apache/solr/core/SolrCore.java

@@ -222,7 +222,7 @@ public final class SolrCore implements SolrInfoBean, SolrMetricProducer, Closeab
   private Counter newSearcherOtherErrorsCounter;
   private final CoreContainer coreContainer;
 
-  private Set<String> metricNames = new HashSet<>();
+  private Set<String> metricNames = ConcurrentHashMap.newKeySet();
 
   public Set<String> getMetricNames() {
     return metricNames;

solr/core/src/java/org/apache/solr/core/SolrInfoBean.java

@@ -66,6 +66,7 @@ public interface SolrInfoBean {
    * Modifiable set of metric names that this component reports (default is null,
    * which means none). If not null then this set is used by {@link #registerMetricName(String)}
    * to capture what metrics names are reported from this component.
+   * <p><b>NOTE: this set has to allow iteration under modifications.</b></p>
    */
   default Set<String> getMetricNames() {
     return null;

solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java

@@ -18,8 +18,8 @@ package org.apache.solr.handler;
 
 import java.lang.invoke.MethodHandles;
 import java.util.Collection;
-import java.util.HashSet;
 import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
 
 import com.codahale.metrics.MetricRegistry;
 import com.google.common.collect.ImmutableList;
@@ -74,7 +74,7 @@ public abstract class RequestHandlerBase implements SolrRequestHandler, SolrInfo
 
   private PluginInfo pluginInfo;
 
-  private Set<String> metricNames = new HashSet<>();
+  private Set<String> metricNames = ConcurrentHashMap.newKeySet();
   private MetricRegistry registry;
 
   @SuppressForbidden(reason = "Need currentTimeMillis, used only for stats output")

solr/core/src/java/org/apache/solr/handler/component/SearchComponent.java

@@ -19,9 +19,9 @@ package org.apache.solr.handler.component;
 import java.io.IOException;
 import java.util.Collections;
 import java.util.HashMap;
-import java.util.HashSet;
 import java.util.Map;
 import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
 
 import com.codahale.metrics.MetricRegistry;
 import org.apache.solr.common.util.NamedList;
@@ -42,7 +42,7 @@ public abstract class SearchComponent implements SolrInfoBean, NamedListInitiali
    */
   private String name = this.getClass().getName();
 
-  protected Set<String> metricNames = new HashSet<>();
+  protected Set<String> metricNames = ConcurrentHashMap.newKeySet();
   protected MetricRegistry registry;
 
   /**

solr/core/src/java/org/apache/solr/highlight/HighlightingPluginBase.java

@@ -16,8 +16,8 @@
  */
 package org.apache.solr.highlight;
 
-import java.util.HashSet;
 import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
 
 import com.codahale.metrics.Counter;
 import com.codahale.metrics.MetricRegistry;
@@ -35,7 +35,7 @@ public abstract class HighlightingPluginBase implements SolrInfoBean, SolrMetric
 {
   protected Counter numRequests;
   protected SolrParams defaults;
-  protected Set<String> metricNames = new HashSet<>(1);
+  protected Set<String> metricNames = ConcurrentHashMap.newKeySet(1);
   protected MetricRegistry registry;
 
   public void init(NamedList args) {

solr/core/src/java/org/apache/solr/search/SolrFieldCacheBean.java

@@ -16,8 +16,8 @@
  */
 package org.apache.solr.search;
 
-import java.util.HashSet;
 import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
 
 import com.codahale.metrics.MetricRegistry;
 import org.apache.solr.core.SolrInfoBean;
@@ -36,7 +36,7 @@ public class SolrFieldCacheBean implements SolrInfoBean, SolrMetricProducer {
   private boolean disableJmxEntryList = Boolean.getBoolean("disableSolrFieldCacheMBeanEntryListJmx");
 
   private MetricRegistry registry;
-  private Set<String> metricNames = new HashSet<>();
+  private Set<String> metricNames = ConcurrentHashMap.newKeySet();
 
   @Override
   public String getName() { return this.getClass().getName(); }

solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java

@@ -25,11 +25,11 @@ import java.util.Collections;
 import java.util.Comparator;
 import java.util.Date;
 import java.util.HashMap;
-import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicLong;
 import java.util.concurrent.atomic.AtomicReference;
@@ -137,7 +137,7 @@ public class SolrIndexSearcher extends IndexSearcher implements Closeable, SolrI
   private final String path;
   private boolean releaseDirectory;
 
-  private Set<String> metricNames = new HashSet<>();
+  private Set<String> metricNames = ConcurrentHashMap.newKeySet();
 
   private static DirectoryReader getReader(SolrCore core, SolrIndexConfig config, DirectoryFactory directoryFactory,
                                            String path) throws IOException {

solr/core/src/java/org/apache/solr/store/blockcache/Metrics.java

@@ -16,8 +16,8 @@
  */
 package org.apache.solr.store.blockcache;
 
-import java.util.HashSet;
 import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.atomic.AtomicLong;
 
 import com.codahale.metrics.MetricRegistry;
@@ -55,8 +55,7 @@ public class Metrics extends SolrCacheBase implements SolrInfoBean, SolrMetricPr
 
   private MetricsMap metricsMap;
   private MetricRegistry registry;
-  private Set<String> metricNames = new HashSet<>();
+  private Set<String> metricNames = ConcurrentHashMap.newKeySet();
-
   private long previous = System.nanoTime();
 
   @Override

solr/core/src/java/org/apache/solr/store/hdfs/HdfsLocalityReporter.java

@@ -19,7 +19,6 @@ package org.apache.solr.store.hdfs;
 import java.io.IOException;
 import java.lang.invoke.MethodHandles;
 import java.util.Arrays;
-import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
@@ -51,7 +50,7 @@ public class HdfsLocalityReporter implements SolrInfoBean, SolrMetricProducer {
   private String hostname;
   private final ConcurrentMap<HdfsDirectory,ConcurrentMap<FileStatus,BlockLocation[]>> cache;
 
-  private final Set<String> metricNames = new HashSet<>();
+  private final Set<String> metricNames = ConcurrentHashMap.newKeySet();
   private MetricRegistry registry;
 
   public HdfsLocalityReporter() {

solr/core/src/java/org/apache/solr/update/UpdateHandler.java

@@ -19,9 +19,9 @@ package org.apache.solr.update;
 
 import java.io.IOException;
 import java.lang.invoke.MethodHandles;
-import java.util.HashSet;
 import java.util.Set;
 import java.util.Vector;
+import java.util.concurrent.ConcurrentHashMap;
 
 import com.codahale.metrics.MetricRegistry;
 import org.apache.solr.core.DirectoryFactory;
@@ -58,7 +58,7 @@ public abstract class UpdateHandler implements SolrInfoBean {
 
   protected final UpdateLog ulog;
 
-  protected Set<String> metricNames = new HashSet<>();
+  protected Set<String> metricNames = ConcurrentHashMap.newKeySet();
   protected MetricRegistry registry;
 
   private void parseEventListeners() {

solr/core/src/java/org/apache/solr/update/UpdateShardHandler.java

@@ -17,8 +17,8 @@
 package org.apache.solr.update;
 
 import java.lang.invoke.MethodHandles;
-import java.util.HashSet;
 import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.ThreadFactory;
 
@@ -66,7 +66,7 @@ public class UpdateShardHandler implements SolrMetricProducer, SolrInfoBean {
 
   private final InstrumentedHttpRequestExecutor httpRequestExecutor;
 
-  private final Set<String> metricNames = new HashSet<>();
+  private final Set<String> metricNames = ConcurrentHashMap.newKeySet();
   private MetricRegistry registry;
 
   public UpdateShardHandler(UpdateShardHandlerConfig cfg) {

solr/core/src/test/org/apache/solr/core/MockInfoBean.java

@@ -16,8 +16,8 @@
  */
 package org.apache.solr.core;
 
-import java.util.HashSet;
 import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
 
 import com.codahale.metrics.MetricRegistry;
 import org.apache.solr.metrics.MetricsMap;
@@ -25,7 +25,7 @@ import org.apache.solr.metrics.SolrMetricManager;
 import org.apache.solr.metrics.SolrMetricProducer;
 
 class MockInfoBean implements SolrInfoBean, SolrMetricProducer {
-  Set<String> metricNames = new HashSet<>();
+  Set<String> metricNames = ConcurrentHashMap.newKeySet();
   MetricRegistry registry;
 
   @Override

solr/core/src/test/org/apache/solr/handler/admin/MBeansHandlerTest.java

@@ -19,14 +19,20 @@ package org.apache.solr.handler.admin;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
+import com.codahale.metrics.MetricRegistry;
 import org.apache.solr.SolrTestCaseJ4;
 import org.apache.solr.common.params.CommonParams;
 import org.apache.solr.common.util.ContentStream;
 import org.apache.solr.common.util.ContentStreamBase;
 import org.apache.solr.common.util.NamedList;
+import org.apache.solr.core.SolrInfoBean;
 import org.apache.solr.request.LocalSolrQueryRequest;
 import org.junit.BeforeClass;
 import org.junit.Test;
@@ -120,4 +126,74 @@ public class MBeansHandlerTest extends SolrTestCaseJ4 {
 
     assertTrue("external entity ignored properly", true);
   }
+
+  boolean runSnapshots;
+
+  @Test
+  public void testMetricsSnapshot() throws Exception {
+    final CountDownLatch counter = new CountDownLatch(500);
+    MetricRegistry registry = new MetricRegistry();
+    Set<String> names = ConcurrentHashMap.newKeySet();
+    SolrInfoBean bean = new SolrInfoBean() {
+      @Override
+      public String getName() {
+        return "foo";
+      }
+
+      @Override
+      public String getDescription() {
+        return "foo";
+      }
+
+      @Override
+      public Category getCategory() {
+        return Category.ADMIN;
+      }
+
+      @Override
+      public Set<String> getMetricNames() {
+        return names;
+      }
+
+      @Override
+      public MetricRegistry getMetricRegistry() {
+        return registry;
+      }
+    };
+    runSnapshots = true;
+    Thread modifier = new Thread(() -> {
+      int i = 0;
+      while (runSnapshots) {
+        bean.registerMetricName("name-" + i++);
+        try {
+          Thread.sleep(31);
+        } catch (InterruptedException e) {
+          runSnapshots = false;
+          break;
+        }
+      }
+    });
+    Thread reader = new Thread(() -> {
+      while (runSnapshots) {
+        try {
+          bean.getMetricsSnapshot();
+        } catch (Exception e) {
+          runSnapshots = false;
+          e.printStackTrace();
+          fail("Exception getting metrics snapshot: " + e.toString());
+        }
+        try {
+          Thread.sleep(53);
+        } catch (InterruptedException e) {
+          runSnapshots = false;
+          break;
+        }
+        counter.countDown();
+      }
+    });
+    modifier.start();
+    reader.start();
+    counter.await(30, TimeUnit.SECONDS);
+    runSnapshots = false;
+  }
 }