GEODE-2215 NPE in ViewCreator thread setting public keys into a NetView NetView creates a ConcurrentHashmap to hold the public keys when it's constructed but it had some methods that were replacing it with a Hashmap. I made the field final and also added checks to avoid putting a null key or value into the map.
Project: http://git-wip-us.apache.org/repos/asf/geode/repo Commit: http://git-wip-us.apache.org/repos/asf/geode/commit/5dfce1bd Tree: http://git-wip-us.apache.org/repos/asf/geode/tree/5dfce1bd Diff: http://git-wip-us.apache.org/repos/asf/geode/diff/5dfce1bd Branch: refs/heads/develop Commit: 5dfce1bd6fe625042abf243dec60098a9394ef27 Parents: f68c416 Author: Bruce Schuchardt <bschucha...@pivotal.io> Authored: Mon Dec 19 09:07:52 2016 -0800 Committer: Bruce Schuchardt <bschucha...@pivotal.io> Committed: Mon Dec 19 09:50:51 2016 -0800 ---------------------------------------------------------------------- .../internal/membership/NetView.java | 50 +- .../internal/membership/NetViewJUnitTest.java | 16 + .../sanctionedDataSerializables.txt | 1064 +++++++++--------- 3 files changed, 581 insertions(+), 549 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/geode/blob/5dfce1bd/geode-core/src/main/java/org/apache/geode/distributed/internal/membership/NetView.java ---------------------------------------------------------------------- diff --git a/geode-core/src/main/java/org/apache/geode/distributed/internal/membership/NetView.java b/geode-core/src/main/java/org/apache/geode/distributed/internal/membership/NetView.java index ca62e20..26b0327 100644 --- a/geode-core/src/main/java/org/apache/geode/distributed/internal/membership/NetView.java +++ b/geode-core/src/main/java/org/apache/geode/distributed/internal/membership/NetView.java @@ -14,21 +14,29 @@ */ package org.apache.geode.distributed.internal.membership; -import java.io.DataInput; -import java.io.DataOutput; -import java.io.IOException; -import java.util.stream.*; -import java.util.*; -import java.util.concurrent.ConcurrentHashMap; - -import org.apache.logging.log4j.Logger; - import org.apache.geode.DataSerializer; import org.apache.geode.distributed.DistributedMember; import org.apache.geode.distributed.internal.DistributionManager; import org.apache.geode.internal.DataSerializableFixedID; import org.apache.geode.internal.InternalDataSerializer; import org.apache.geode.internal.Version; +import org.apache.logging.log4j.Logger; + +import java.io.DataInput; +import java.io.DataOutput; +import java.io.IOException; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.Collections; +import java.util.HashSet; +import java.util.Iterator; +import java.util.List; +import java.util.Map; +import java.util.Random; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.stream.Collectors; /** * The NetView class represents a membership view. Note that this class is not synchronized, so take @@ -41,7 +49,7 @@ public class NetView implements DataSerializableFixedID { private int viewId; private List<InternalDistributedMember> members; // TODO this should be a List - private Map<InternalDistributedMember, Object> publicKeys = new ConcurrentHashMap<>(); + private final Map<InternalDistributedMember, Object> publicKeys = new ConcurrentHashMap<>(); private int[] failureDetectionPorts = new int[10]; private Set<InternalDistributedMember> shutdownMembers; private Set<InternalDistributedMember> crashedMembers; @@ -113,7 +121,7 @@ public class NetView implements DataSerializableFixedID { other.failureDetectionPorts.length); this.shutdownMembers = new HashSet<>(other.shutdownMembers); this.crashedMembers = new HashSet<>(other.crashedMembers); - this.publicKeys = new HashMap<>(other.publicKeys); + this.publicKeys.putAll(other.publicKeys); } public NetView(InternalDistributedMember creator, int viewId, @@ -146,14 +154,19 @@ public class NetView implements DataSerializableFixedID { } public void setPublicKey(InternalDistributedMember mbr, Object key) { - publicKeys.put(mbr, key); + if (mbr != null && key != null) { + publicKeys.put(mbr, key); + } } public void setPublicKeys(NetView otherView) { - this.publicKeys.putAll(otherView.publicKeys); + if (otherView.publicKeys != null) { + this.publicKeys.putAll(otherView.publicKeys); + } } + public int[] getFailureDetectionPorts() { return this.failureDetectionPorts; } @@ -558,10 +571,10 @@ public class NetView implements DataSerializableFixedID { if (other == this) { return true; } - if (!(other instanceof NetView)) { - return false; + if (other instanceof NetView) { + return this.members.equals(((NetView) other).getMembers()); } - return this.members.equals(((NetView) other).getMembers()); + return false; } @Override @@ -591,7 +604,10 @@ public class NetView implements DataSerializableFixedID { shutdownMembers = InternalDataSerializer.readHashSet(in); crashedMembers = InternalDataSerializer.readHashSet(in); failureDetectionPorts = DataSerializer.readIntArray(in); - publicKeys = DataSerializer.readHashMap(in); + Map pubkeys = DataSerializer.readHashMap(in); + if (pubkeys != null) { + publicKeys.putAll(pubkeys); + } } /** this will deserialize as an ArrayList */ http://git-wip-us.apache.org/repos/asf/geode/blob/5dfce1bd/geode-core/src/test/java/org/apache/geode/distributed/internal/membership/NetViewJUnitTest.java ---------------------------------------------------------------------- diff --git a/geode-core/src/test/java/org/apache/geode/distributed/internal/membership/NetViewJUnitTest.java b/geode-core/src/test/java/org/apache/geode/distributed/internal/membership/NetViewJUnitTest.java index 715da47..fef77de 100755 --- a/geode-core/src/test/java/org/apache/geode/distributed/internal/membership/NetViewJUnitTest.java +++ b/geode-core/src/test/java/org/apache/geode/distributed/internal/membership/NetViewJUnitTest.java @@ -193,6 +193,22 @@ public class NetViewJUnitTest { assertEquals(100, view.getNewMembers(copy).size()); } + @Test + public void testNullPublicKeysNotRetained() throws Exception { + NetView view = new NetView(members.get(0), 2, new ArrayList<>(members)); + setFailureDetectionPorts(view); + + NetView newView = new NetView(view, 3); + for (InternalDistributedMember member : view.getMembers()) { + view.setPublicKey(member, null); + } + newView.setPublicKeys(view); + for (InternalDistributedMember member : view.getMembers()) { + assertNull(newView.getPublicKey(member)); + assertNull(view.getPublicKey(member)); + } + } + /** * Test that failed weight calculations are correctly performed. See bug #47342 */