From 806232941188ce8e87e6112014123b65bc2e0be4 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Fri, 18 Dec 2015 08:17:37 -0500 Subject: [PATCH] Fix ConcurrentModificationException from nodes info and nodes stats A ConcurrentModificationException can arise on an Elasticsearch cluster running OpenJDK 8 based JVMs from concurrent requests to the nodes info, nodes stats and cat plugins endpoints. The issue can arise because PlugsinInfo#getInfos currently performs a sort of its backing list. This method is used in each of those endpoints and two concurrent requests will cause this backing list to be sorted concurrently. Since sorting the backing list modifies it, and concurrent modifications are bad, the sort implementation in OpenJDK 8 added protection against concurrent modifications. This commit addresses this issue by removing the sort from the Plugins#getInfos method, but still preserves the sorted result from this method. Closes #15537 --- .../admin/cluster/node/info/PluginsInfo.java | 28 +++++------- .../plugins/PluginInfoTests.java | 44 ++++++++++++++++++- 2 files changed, 55 insertions(+), 17 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/action/admin/cluster/node/info/PluginsInfo.java b/core/src/main/java/org/elasticsearch/action/admin/cluster/node/info/PluginsInfo.java index 927a79b6639c4..e80605fb6a876 100644 --- a/core/src/main/java/org/elasticsearch/action/admin/cluster/node/info/PluginsInfo.java +++ b/core/src/main/java/org/elasticsearch/action/admin/cluster/node/info/PluginsInfo.java @@ -28,38 +28,33 @@ import org.elasticsearch.plugins.PluginInfo; import java.io.IOException; -import java.util.ArrayList; -import java.util.Collections; +import java.util.Arrays; import java.util.Comparator; import java.util.List; +import java.util.Set; +import java.util.TreeSet; public class PluginsInfo implements Streamable, ToXContent { static final class Fields { static final XContentBuilderString PLUGINS = new XContentBuilderString("plugins"); } - private List infos; + private Set infos; public PluginsInfo() { - infos = new ArrayList<>(); - } - - public PluginsInfo(int size) { - infos = new ArrayList<>(size); - } - - /** - * @return an ordered list based on plugins name - */ - public List getInfos() { - Collections.sort(infos, new Comparator() { + infos = new TreeSet<>(new Comparator() { @Override public int compare(final PluginInfo o1, final PluginInfo o2) { return o1.getName().compareTo(o2.getName()); } }); + } - return infos; + /** + * @return an ordered list based on plugins name + */ + public List getInfos() { + return Arrays.asList(infos.toArray(new PluginInfo[infos.size()])); } public void add(PluginInfo info) { @@ -75,6 +70,7 @@ public static PluginsInfo readPluginsInfo(StreamInput in) throws IOException { @Override public void readFrom(StreamInput in) throws IOException { int plugins_size = in.readInt(); + infos.clear(); for (int i = 0; i < plugins_size; i++) { infos.add(PluginInfo.readFromStream(in)); } diff --git a/core/src/test/java/org/elasticsearch/plugins/PluginInfoTests.java b/core/src/test/java/org/elasticsearch/plugins/PluginInfoTests.java index 01c1bbfc6e005..c354d7f4ede8b 100644 --- a/core/src/test/java/org/elasticsearch/plugins/PluginInfoTests.java +++ b/core/src/test/java/org/elasticsearch/plugins/PluginInfoTests.java @@ -28,8 +28,12 @@ import java.io.OutputStream; import java.nio.file.Files; import java.nio.file.Path; +import java.util.ArrayList; +import java.util.ConcurrentModificationException; import java.util.List; import java.util.Properties; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.atomic.AtomicBoolean; import static org.elasticsearch.common.util.CollectionUtils.eagerTransform; import static org.hamcrest.Matchers.contains; @@ -273,7 +277,7 @@ public void testReadFromPropertiesSitePluginWithoutSite() throws Exception { } public void testPluginListSorted() { - PluginsInfo pluginsInfo = new PluginsInfo(5); + PluginsInfo pluginsInfo = new PluginsInfo(); pluginsInfo.add(new PluginInfo("c", "foo", true, "dummy", true, "dummyclass", true)); pluginsInfo.add(new PluginInfo("b", "foo", true, "dummy", true, "dummyclass", true)); pluginsInfo.add(new PluginInfo("e", "foo", true, "dummy", true, "dummyclass", true)); @@ -289,4 +293,42 @@ public String apply(PluginInfo input) { }); assertThat(names, contains("a", "b", "c", "d", "e")); } + + public void testConcurrentModificationsAreAvoided() throws InterruptedException { + final PluginsInfo pluginsInfo = new PluginsInfo(); + int numberOfPlugins = randomIntBetween(128, 256); + for (int i = 0; i < numberOfPlugins; i++) { + pluginsInfo.add(new PluginInfo("name", "description", false, "version", true, "classname", true)); + } + + int randomNumberOfThreads = randomIntBetween(2, 8); + final int numberOfAttempts = randomIntBetween(2048, 4096); + final CountDownLatch latch = new CountDownLatch(1 + randomNumberOfThreads); + List threads = new ArrayList<>(randomNumberOfThreads); + final AtomicBoolean cme = new AtomicBoolean(); + for (int i = 0; i < randomNumberOfThreads; i++) { + Thread thread = new Thread(new Runnable() { + @Override + public void run() { + latch.countDown(); + for (int j = 0; j < numberOfAttempts; j++) { + try { + pluginsInfo.getInfos(); + } catch (ConcurrentModificationException e) { + cme.set(true); + } + } + } + }); + threads.add(thread); + thread.start(); + } + + latch.countDown(); + for (Thread thread : threads) { + thread.join(); + } + + assertFalse(cme.get()); + } }