From 871d1b48853bc3b31376a00d8b5a74edbd64e857 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Fri, 8 Jan 2016 12:10:03 -0500 Subject: [PATCH] Remove and forbid use of j.u.c.ThreadLocalRandom This commit removes and now forbids all uses of java.util.concurrent.ThreadLocalRandom across the codebase. The underlying issue with ThreadLocalRandom is that it can not be seeded. This means that if ThreadLocalRandom is used in production code, then tests that cover any code path containing ThreadLocalRandom will be prevented from being reproducible by use of ThreadLocalRandom. Instead, using org.elasticsearch.common.random.Randomness#get will give reproducible sources of random when running under tests and otherwise still give an instance of ThreadLocalRandom when running as production code. --- .../main/resources/forbidden/all-signatures.txt | 4 +++- .../query/TransportValidateQueryAction.java | 4 ++-- .../cluster/routing/IndexRoutingTable.java | 4 ++-- .../cluster/routing/IndexShardRoutingTable.java | 4 ++-- .../java/org/elasticsearch/common/Randomness.java | 1 + .../node/internal/InternalSettingsPreparer.java | 4 ++-- .../JavaScriptScriptMultiThreadedTests.java | 14 +++++++------- .../python/PythonScriptMultiThreadedTests.java | 14 +++++++------- 8 files changed, 26 insertions(+), 23 deletions(-) diff --git a/buildSrc/src/main/resources/forbidden/all-signatures.txt b/buildSrc/src/main/resources/forbidden/all-signatures.txt index c1e65cbaf22..76cd83cdede 100644 --- a/buildSrc/src/main/resources/forbidden/all-signatures.txt +++ b/buildSrc/src/main/resources/forbidden/all-signatures.txt @@ -125,4 +125,6 @@ java.util.Collections#EMPTY_MAP java.util.Collections#EMPTY_SET java.util.Collections#shuffle(java.util.List) @ Use java.util.Collections#shuffle(java.util.List, java.util.Random) with a reproducible source of randomness -java.util.Random#() @ Use org.elasticsearch.common.random.Randomness#create for reproducible sources of randomness +@defaultMessage Use org.elasticsearch.common.Randomness#get for reproducible sources of randomness +java.util.Random#() +java.util.concurrent.ThreadLocalRandom \ No newline at end of file diff --git a/core/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java b/core/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java index 326dbc01289..225ee326b95 100644 --- a/core/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java +++ b/core/src/main/java/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java @@ -37,6 +37,7 @@ import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver; import org.elasticsearch.cluster.routing.GroupShardsIterator; import org.elasticsearch.cluster.routing.ShardRouting; import org.elasticsearch.common.ParsingException; +import org.elasticsearch.common.Randomness; import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.util.BigArrays; @@ -59,7 +60,6 @@ import java.util.ArrayList; import java.util.List; import java.util.Map; import java.util.Set; -import java.util.concurrent.ThreadLocalRandom; import java.util.concurrent.atomic.AtomicReferenceArray; /** @@ -108,7 +108,7 @@ public class TransportValidateQueryAction extends TransportBroadcastAction> routingMap = indexNameExpressionResolver.resolveSearchRouting(clusterState, Integer.toString(ThreadLocalRandom.current().nextInt(1000)), request.indices()); + Map> routingMap = indexNameExpressionResolver.resolveSearchRouting(clusterState, Integer.toString(Randomness.get().nextInt(1000)), request.indices()); return clusterService.operationRouting().searchShards(clusterState, concreteIndices, routingMap, "_local"); } diff --git a/core/src/main/java/org/elasticsearch/cluster/routing/IndexRoutingTable.java b/core/src/main/java/org/elasticsearch/cluster/routing/IndexRoutingTable.java index bcf489c6a2c..bb186a64a8c 100644 --- a/core/src/main/java/org/elasticsearch/cluster/routing/IndexRoutingTable.java +++ b/core/src/main/java/org/elasticsearch/cluster/routing/IndexRoutingTable.java @@ -26,6 +26,7 @@ import org.apache.lucene.util.CollectionUtil; import org.elasticsearch.cluster.AbstractDiffable; import org.elasticsearch.cluster.metadata.IndexMetaData; import org.elasticsearch.cluster.metadata.MetaData; +import org.elasticsearch.common.Randomness; import org.elasticsearch.common.collect.ImmutableOpenIntMap; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; @@ -39,7 +40,6 @@ import java.util.HashSet; import java.util.Iterator; import java.util.List; import java.util.Set; -import java.util.concurrent.ThreadLocalRandom; /** * The {@link IndexRoutingTable} represents routing information for a single @@ -71,7 +71,7 @@ public class IndexRoutingTable extends AbstractDiffable imple IndexRoutingTable(String index, ImmutableOpenIntMap shards) { this.index = index; - this.shuffler = new RotationShardShuffler(ThreadLocalRandom.current().nextInt()); + this.shuffler = new RotationShardShuffler(Randomness.get().nextInt()); this.shards = shards; List allActiveShards = new ArrayList<>(); for (IntObjectCursor cursor : shards) { diff --git a/core/src/main/java/org/elasticsearch/cluster/routing/IndexShardRoutingTable.java b/core/src/main/java/org/elasticsearch/cluster/routing/IndexShardRoutingTable.java index d425b63b34c..bcdb7a43fef 100644 --- a/core/src/main/java/org/elasticsearch/cluster/routing/IndexShardRoutingTable.java +++ b/core/src/main/java/org/elasticsearch/cluster/routing/IndexShardRoutingTable.java @@ -21,6 +21,7 @@ package org.elasticsearch.cluster.routing; import org.elasticsearch.cluster.node.DiscoveryNode; import org.elasticsearch.cluster.node.DiscoveryNodes; +import org.elasticsearch.common.Randomness; import org.elasticsearch.common.collect.MapBuilder; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; @@ -36,7 +37,6 @@ import java.util.LinkedList; import java.util.List; import java.util.Map; import java.util.Set; -import java.util.concurrent.ThreadLocalRandom; import static java.util.Collections.emptyMap; @@ -73,7 +73,7 @@ public class IndexShardRoutingTable implements Iterable { IndexShardRoutingTable(ShardId shardId, List shards) { this.shardId = shardId; - this.shuffler = new RotationShardShuffler(ThreadLocalRandom.current().nextInt()); + this.shuffler = new RotationShardShuffler(Randomness.get().nextInt()); this.shards = Collections.unmodifiableList(shards); ShardRouting primary = null; diff --git a/core/src/main/java/org/elasticsearch/common/Randomness.java b/core/src/main/java/org/elasticsearch/common/Randomness.java index dbfa8034b99..7f71afc1c70 100644 --- a/core/src/main/java/org/elasticsearch/common/Randomness.java +++ b/core/src/main/java/org/elasticsearch/common/Randomness.java @@ -109,6 +109,7 @@ public final class Randomness { } } + @SuppressForbidden(reason = "ThreadLocalRandom is okay when not running tests") private static Random getWithoutSeed() { assert currentMethod == null && getRandomMethod == null : "running under tests but tried to create non-reproducible random"; return ThreadLocalRandom.current(); diff --git a/core/src/main/java/org/elasticsearch/node/internal/InternalSettingsPreparer.java b/core/src/main/java/org/elasticsearch/node/internal/InternalSettingsPreparer.java index 11db520ed7d..53b86e4dce4 100644 --- a/core/src/main/java/org/elasticsearch/node/internal/InternalSettingsPreparer.java +++ b/core/src/main/java/org/elasticsearch/node/internal/InternalSettingsPreparer.java @@ -22,6 +22,7 @@ package org.elasticsearch.node.internal; import org.elasticsearch.bootstrap.BootstrapInfo; import org.elasticsearch.cluster.ClusterName; import org.elasticsearch.common.Booleans; +import org.elasticsearch.common.Randomness; import org.elasticsearch.common.Strings; import org.elasticsearch.common.cli.Terminal; import org.elasticsearch.common.collect.Tuple; @@ -41,7 +42,6 @@ import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; -import java.util.concurrent.ThreadLocalRandom; import static org.elasticsearch.common.Strings.cleanPath; import static org.elasticsearch.common.settings.Settings.settingsBuilder; @@ -207,7 +207,7 @@ public class InternalSettingsPreparer { name = reader.readLine(); } } - int index = ThreadLocalRandom.current().nextInt(names.size()); + int index = Randomness.get().nextInt(names.size()); return names.get(index); } catch (IOException e) { throw new RuntimeException("Could not read node names list", e); diff --git a/plugins/lang-javascript/src/test/java/org/elasticsearch/script/javascript/JavaScriptScriptMultiThreadedTests.java b/plugins/lang-javascript/src/test/java/org/elasticsearch/script/javascript/JavaScriptScriptMultiThreadedTests.java index 2aa6e13a99f..b6be9f6dde0 100644 --- a/plugins/lang-javascript/src/test/java/org/elasticsearch/script/javascript/JavaScriptScriptMultiThreadedTests.java +++ b/plugins/lang-javascript/src/test/java/org/elasticsearch/script/javascript/JavaScriptScriptMultiThreadedTests.java @@ -19,6 +19,7 @@ package org.elasticsearch.script.javascript; +import org.elasticsearch.common.Randomness; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.script.CompiledScript; import org.elasticsearch.script.ExecutableScript; @@ -30,7 +31,6 @@ import java.util.HashMap; import java.util.Map; import java.util.concurrent.CountDownLatch; import java.util.concurrent.CyclicBarrier; -import java.util.concurrent.ThreadLocalRandom; import java.util.concurrent.atomic.AtomicBoolean; import static org.hamcrest.Matchers.equalTo; @@ -53,8 +53,8 @@ public class JavaScriptScriptMultiThreadedTests extends ESTestCase { public void run() { try { barrier.await(); - long x = ThreadLocalRandom.current().nextInt(); - long y = ThreadLocalRandom.current().nextInt(); + long x = Randomness.get().nextInt(); + long y = Randomness.get().nextInt(); long addition = x + y; Map vars = new HashMap(); vars.put("x", x); @@ -95,12 +95,12 @@ public class JavaScriptScriptMultiThreadedTests extends ESTestCase { public void run() { try { barrier.await(); - long x = ThreadLocalRandom.current().nextInt(); + long x = Randomness.get().nextInt(); Map vars = new HashMap(); vars.put("x", x); ExecutableScript script = se.executable(new CompiledScript(ScriptService.ScriptType.INLINE, "testExecutableNoRuntimeParams", "js", compiled), vars); for (int i = 0; i < 100000; i++) { - long y = ThreadLocalRandom.current().nextInt(); + long y = Randomness.get().nextInt(); long addition = x + y; script.setNextVar("y", y); long result = ((Number) script.run()).longValue(); @@ -139,8 +139,8 @@ public class JavaScriptScriptMultiThreadedTests extends ESTestCase { barrier.await(); Map runtimeVars = new HashMap(); for (int i = 0; i < 100000; i++) { - long x = ThreadLocalRandom.current().nextInt(); - long y = ThreadLocalRandom.current().nextInt(); + long x = Randomness.get().nextInt(); + long y = Randomness.get().nextInt(); long addition = x + y; runtimeVars.put("x", x); runtimeVars.put("y", y); diff --git a/plugins/lang-python/src/test/java/org/elasticsearch/script/python/PythonScriptMultiThreadedTests.java b/plugins/lang-python/src/test/java/org/elasticsearch/script/python/PythonScriptMultiThreadedTests.java index 06d3da03ab8..f24cc889a70 100644 --- a/plugins/lang-python/src/test/java/org/elasticsearch/script/python/PythonScriptMultiThreadedTests.java +++ b/plugins/lang-python/src/test/java/org/elasticsearch/script/python/PythonScriptMultiThreadedTests.java @@ -19,6 +19,7 @@ package org.elasticsearch.script.python; +import org.elasticsearch.common.Randomness; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.script.CompiledScript; import org.elasticsearch.script.ExecutableScript; @@ -30,7 +31,6 @@ import java.util.HashMap; import java.util.Map; import java.util.concurrent.CountDownLatch; import java.util.concurrent.CyclicBarrier; -import java.util.concurrent.ThreadLocalRandom; import java.util.concurrent.atomic.AtomicBoolean; import static org.hamcrest.Matchers.equalTo; @@ -55,8 +55,8 @@ public class PythonScriptMultiThreadedTests extends ESTestCase { public void run() { try { barrier.await(); - long x = ThreadLocalRandom.current().nextInt(); - long y = ThreadLocalRandom.current().nextInt(); + long x = Randomness.get().nextInt(); + long y = Randomness.get().nextInt(); long addition = x + y; Map vars = new HashMap(); vars.put("x", x); @@ -97,13 +97,13 @@ public class PythonScriptMultiThreadedTests extends ESTestCase { // @Override public void run() { // try { // barrier.await(); -// long x = ThreadLocalRandom.current().nextInt(); +// long x = Randomness.get().nextInt(); // Map vars = new HashMap(); // vars.put("x", x); // ExecutableScript script = se.executable(compiled, vars); // Map runtimeVars = new HashMap(); // for (int i = 0; i < 100000; i++) { -// long y = ThreadLocalRandom.current().nextInt(); +// long y = Randomness.get().nextInt(); // long addition = x + y; // runtimeVars.put("y", y); // long result = ((Number) script.run(runtimeVars)).longValue(); @@ -143,8 +143,8 @@ public class PythonScriptMultiThreadedTests extends ESTestCase { barrier.await(); Map runtimeVars = new HashMap(); for (int i = 0; i < 10000; i++) { - long x = ThreadLocalRandom.current().nextInt(); - long y = ThreadLocalRandom.current().nextInt(); + long x = Randomness.get().nextInt(); + long y = Randomness.get().nextInt(); long addition = x + y; runtimeVars.put("x", x); runtimeVars.put("y", y);