Forráskód Böngészése

Avoid using assertOnce things as map keys (#96565)

It's legitimate to wrap the delegate twice, with two different
assertOnce calls, which would yield different objects if and only if
assertions are enabled. So we'd better not ever use these things as map
keys etc.
David Turner 2 éve
szülő
commit
eca1e4f831

+ 14 - 0
libs/core/src/main/java/org/elasticsearch/core/Releasables.java

@@ -146,6 +146,20 @@ public enum Releasables {
                 public String toString() {
                     return delegate.toString();
                 }
+
+                @Override
+                public int hashCode() {
+                    // It's legitimate to wrap the delegate twice, with two different assertOnce calls, which would yield different objects
+                    // if and only if assertions are enabled. So we'd better not ever use these things as map keys etc.
+                    throw new AssertionError("almost certainly a mistake to need the hashCode() of a one-shot Releasable");
+                }
+
+                @Override
+                public boolean equals(Object obj) {
+                    // It's legitimate to wrap the delegate twice, with two different assertOnce calls, which would yield different objects
+                    // if and only if assertions are enabled. So we'd better not ever use these things as map keys etc.
+                    throw new AssertionError("almost certainly a mistake to compare a one-shot Releasable for equality");
+                }
             };
         } else {
             return delegate;

+ 14 - 0
server/src/main/java/org/elasticsearch/action/ActionListener.java

@@ -343,6 +343,20 @@ public interface ActionListener<Response> {
                 public String toString() {
                     return delegate.toString();
                 }
+
+                @Override
+                public int hashCode() {
+                    // It's legitimate to wrap the delegate twice, with two different assertOnce calls, which would yield different objects
+                    // if and only if assertions are enabled. So we'd better not ever use these things as map keys etc.
+                    throw new AssertionError("almost certainly a mistake to need the hashCode() of a one-shot ActionListener");
+                }
+
+                @Override
+                public boolean equals(Object obj) {
+                    // It's legitimate to wrap the delegate twice, with two different assertOnce calls, which would yield different objects
+                    // if and only if assertions are enabled. So we'd better not ever use these things as map keys etc.
+                    throw new AssertionError("almost certainly a mistake to compare a one-shot ActionListener for equality");
+                }
             };
         } else {
             return delegate;

+ 4 - 4
server/src/test/java/org/elasticsearch/action/support/replication/BroadcastReplicationTests.java

@@ -29,7 +29,6 @@ import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.network.NetworkService;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.util.PageCacheRecycler;
-import org.elasticsearch.common.util.concurrent.ConcurrentCollections;
 import org.elasticsearch.core.IOUtils;
 import org.elasticsearch.core.Tuple;
 import org.elasticsearch.index.shard.ShardId;
@@ -51,11 +50,11 @@ import org.junit.Before;
 import org.junit.BeforeClass;
 
 import java.io.IOException;
+import java.util.ArrayList;
 import java.util.Collections;
 import java.util.Date;
 import java.util.HashSet;
 import java.util.List;
-import java.util.Set;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeUnit;
 
@@ -237,8 +236,9 @@ public class BroadcastReplicationTests extends ESTestCase {
         BaseBroadcastResponse,
         BasicReplicationRequest,
         ReplicationResponse> {
-        protected final Set<Tuple<ShardId, ActionListener<ReplicationResponse>>> capturedShardRequests = ConcurrentCollections
-            .newConcurrentSet();
+        protected final List<Tuple<ShardId, ActionListener<ReplicationResponse>>> capturedShardRequests = Collections.synchronizedList(
+            new ArrayList<>()
+        );
 
         TestBroadcastReplicationAction(
             ClusterService clusterService,