This is an automated email from the ASF dual-hosted git repository.
burcham pushed a commit to branch support/1.14
in repository https://gitbox.apache.org/repos/asf/geode.git
commit ff947f1328f87ec8a8f4a914aea80f3d59eaef59
Author: Kamilla Aslami <kasl...@vmware.com>
AuthorDate: Tue Mar 2 11:50:00 2021 -0600
GEODE-7245: Remove most uses of latestViewReadLock in GMSMembership (#6037)
* Remove most uses of latestViewReadLock in GMSMembership
* make MembershipView immutable
* Constructor consolidation and better naming of add/remove
* Tighten constructor chaining
Co-authored-by: Bill Burcham <bill.burc...@gmail.com>
(cherry picked from commit 477ffba8e155dca0bdeeefd312b6b32b4b481100)
---
.../internal/membership/api/MembershipView.java | 147 +++-------
.../internal/membership/gms/GMSMembership.java | 313 ++++++++-------------
2 files changed, 164 insertions(+), 296 deletions(-)
diff --git
a/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/api/MembershipView.java
b/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/api/MembershipView.java
index 22a7691..902837c 100644
---
a/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/api/MembershipView.java
+++
b/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/api/MembershipView.java
@@ -14,75 +14,53 @@
*/
package org.apache.geode.distributed.internal.membership.api;
+import static java.util.stream.Collectors.toList;
+
import java.util.ArrayList;
-import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
+import java.util.stream.Stream;
/**
- * The MembershipView class represents a membership view. MembershipViews are
typically
- * unmodifiable though you an create and manipulate one for local usel A
MembershipView
- * defines who is in the cluster and knows which node created the view. It
also knows which
- * members left or were removed when the view was created. MemberIdentifiers
in the view
- * are marked with the viewId of the MembershipView in which they joined the
cluster.
+ * The MembershipView class represents a membership view. A MembershipView
defines who is in the
+ * cluster and knows which node created the view. It also knows which members
left or were removed
+ * when the view was created. MemberIdentifiers in the view are marked with
the viewId of the
+ * MembershipView in which they joined the cluster.
*/
public class MembershipView<ID extends MemberIdentifier> {
- private int viewId;
- private List<ID> members;
- private Set<ID> shutdownMembers;
- private Set<ID> crashedMembers;
- private ID creator;
- private Set<ID> hashedMembers;
- private volatile boolean unmodifiable;
-
+ private final int viewId;
+ private final List<ID> members;
+ private final Set<ID> shutdownMembers;
+ private final Set<ID> crashedMembers;
+ private final ID creator;
+ private final Set<ID> hashedMembers;
public MembershipView() {
- viewId = -1;
- members = new ArrayList<>(0);
- this.hashedMembers = new HashSet<>(members);
- shutdownMembers = Collections.emptySet();
- crashedMembers = new HashSet<>();
- creator = null;
+ this(null, -1, Collections.emptyList());
}
- public MembershipView(ID creator, int viewId,
- List<ID> members) {
- this.viewId = viewId;
- this.members = new ArrayList<>(members);
- hashedMembers = new HashSet<>(this.members);
- shutdownMembers = new HashSet<>();
- crashedMembers = Collections.emptySet();
- this.creator = creator;
- }
-
- /**
- * Create a new view with the contents of the given view and the specified
view ID
- */
- public MembershipView(MembershipView<ID> other, int viewId) {
- this.creator = other.creator;
- this.viewId = viewId;
- this.members = new ArrayList<>(other.members);
- this.hashedMembers = new HashSet<>(other.members);
- this.shutdownMembers = new HashSet<>(other.shutdownMembers);
- this.crashedMembers = new HashSet<>(other.crashedMembers);
+ public MembershipView(final ID creator, final int viewId, final List<ID>
members) {
+ this(creator, viewId, members, Collections.emptySet(),
Collections.emptySet());
}
- public MembershipView(ID creator, int viewId,
- List<ID> mbrs, Set<ID> shutdowns,
- Set<ID> crashes) {
+ public MembershipView(final ID creator, final int viewId, final List<ID>
members,
+ final Set<ID> shutdowns, final Set<ID> crashes) {
this.creator = creator;
this.viewId = viewId;
- this.members = mbrs;
- this.hashedMembers = new HashSet<>(mbrs);
- this.shutdownMembers = shutdowns;
- this.crashedMembers = crashes;
- }
- public void makeUnmodifiable() {
- unmodifiable = true;
+ /*
+ * Copy each collection, then store the ref to the unmodifiable
+ * wrapper so we can expose it.
+ */
+ this.members = Collections.unmodifiableList(new ArrayList<>(members));
+ this.shutdownMembers = Collections.unmodifiableSet(new
HashSet<>(shutdowns));
+ this.crashedMembers = Collections.unmodifiableSet(new HashSet<>(crashes));
+
+ // make this unmodifiable for good measure (even though we don't expose it)
+ this.hashedMembers = Collections.unmodifiableSet(new HashSet<>(members));
}
public int getViewId() {
@@ -93,55 +71,24 @@ public class MembershipView<ID extends MemberIdentifier> {
return this.creator;
}
- public void setCreator(ID creator) {
- this.creator = creator;
- }
-
- public void setViewId(int viewId) {
- this.viewId = viewId;
- }
-
-
-
public List<ID> getMembers() {
- return Collections.unmodifiableList(this.members);
- }
-
- /**
- * return members that are i this view but not the given old view
- */
- public List<ID> getNewMembers(MembershipView<ID> olderView) {
- List<ID> result = new ArrayList<>(members);
- result.removeAll(olderView.getMembers());
- return result;
+ return members;
}
public Object get(int i) {
return this.members.get(i);
}
- public void add(ID mbr) {
- if (unmodifiable) {
- throw new IllegalStateException("this membership view is not
modifiable");
- }
- this.hashedMembers.add(mbr);
- this.members.add(mbr);
+ public MembershipView<ID> createNewViewWithMember(ID member) {
+ return new MembershipView<>(creator, viewId,
+ Stream.concat(members.stream(), Stream.of(member)).collect(toList()),
shutdownMembers,
+ crashedMembers);
}
- public boolean remove(ID mbr) {
- if (unmodifiable) {
- throw new IllegalStateException("this membership view is not
modifiable");
- }
- this.hashedMembers.remove(mbr);
- return this.members.remove(mbr);
- }
-
- public void removeAll(Collection<ID> ids) {
- if (unmodifiable) {
- throw new IllegalStateException("this membership view is not
modifiable");
- }
- this.hashedMembers.removeAll(ids);
- ids.forEach(this::remove);
+ public MembershipView<ID> createNewViewWithoutMember(final ID member) {
+ return new MembershipView<>(creator, viewId,
+ members.stream().filter(m -> !m.equals(member)).collect(toList()),
shutdownMembers,
+ crashedMembers);
}
public boolean contains(MemberIdentifier mbr) {
@@ -166,7 +113,7 @@ public class MembershipView<ID extends MemberIdentifier> {
* Returns the ID from this view that is equal to the argument. If no such
ID exists the argument
* is returned.
*/
- public synchronized ID getCanonicalID(ID id) {
+ public ID getCanonicalID(ID id) {
if (hashedMembers.contains(id)) {
for (ID m : this.members) {
if (id.equals(m)) {
@@ -178,7 +125,6 @@ public class MembershipView<ID extends MemberIdentifier> {
}
-
public ID getCoordinator() {
for (ID addr : members) {
if (addr.preferredForCoordinator()) {
@@ -191,10 +137,6 @@ public class MembershipView<ID extends MemberIdentifier> {
return null;
}
- public Set<ID> getShutdownMembers() {
- return this.shutdownMembers;
- }
-
public Set<ID> getCrashedMembers() {
return this.crashedMembers;
}
@@ -206,8 +148,9 @@ public class MembershipView<ID extends MemberIdentifier> {
sb.append("View[").append(creator).append('|').append(viewId).append("]
members: [");
boolean first = true;
for (ID mbr : this.members) {
- if (!first)
+ if (!first) {
sb.append(", ");
+ }
sb.append(mbr);
if (mbr == lead) {
sb.append("{lead}");
@@ -218,8 +161,9 @@ public class MembershipView<ID extends MemberIdentifier> {
sb.append("] shutdown: [");
first = true;
for (ID mbr : this.shutdownMembers) {
- if (!first)
+ if (!first) {
sb.append(", ");
+ }
sb.append(mbr);
first = false;
}
@@ -228,8 +172,9 @@ public class MembershipView<ID extends MemberIdentifier> {
sb.append("] crashed: [");
first = true;
for (ID mbr : this.crashedMembers) {
- if (!first)
+ if (!first) {
sb.append(", ");
+ }
sb.append(mbr);
first = false;
}
@@ -239,7 +184,7 @@ public class MembershipView<ID extends MemberIdentifier> {
}
@Override
- public synchronized boolean equals(Object other) {
+ public boolean equals(Object other) {
if (other == this) {
return true;
}
@@ -250,7 +195,7 @@ public class MembershipView<ID extends MemberIdentifier> {
}
@Override
- public synchronized int hashCode() {
+ public int hashCode() {
return this.members.hashCode();
}
diff --git
a/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMembership.java
b/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMembership.java
index 3d13e1b..3c05988 100644
---
a/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMembership.java
+++
b/geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMembership.java
@@ -223,12 +223,13 @@ public class GMSMembership<ID extends MemberIdentifier>
implements Membership<ID
/**
* This is the latest view (ordered list of IDs) that has been installed
*
- * All accesses to this object are protected via {@link #latestViewLock}
+ * Writing to this object is protected via {@link #latestViewWriteLock}
*/
private volatile MembershipView<ID> latestView = new MembershipView<>();
/**
- * This is the lock for protecting access to latestView
+ * This is the lock for protecting access to latestView and modification of
data used in
+ * {@link #processView} method
*
* @see #latestView
*/
@@ -263,27 +264,36 @@ public class GMSMembership<ID extends MemberIdentifier>
implements Membership<ID
*
* Members are removed after {@link #SHUNNED_SUNSET} seconds have passed.
*
- * Accesses to this list needs to be under the read or write lock of {@link
#latestViewLock}
+ * Writing to this list needs to be under {@link #latestViewWriteLock}
*
* @see System#currentTimeMillis()
*/
- // protected final Set shunnedMembers = Collections.synchronizedSet(new
HashSet());
private final Map<ID, Long> shunnedMembers = new ConcurrentHashMap<>();
/**
* Members that have sent a shutdown message. This is used to suppress
suspect processing that
* otherwise becomes pretty aggressive when a member is shutting down.
*/
- private final Map<ID, Object> shutdownMembers = new BoundedLinkedHashMap<>();
+ private final Set<ID> shutdownMembers = Collections.newSetFromMap(new
BoundedLinkedHashMap<>());
+
+ /**
+ * This is the lock for protecting access to shutdownMembers
+ *
+ * @see #shutdownMembers
+ */
+ private final ReadWriteLock shutdownMembersLock = new
ReentrantReadWriteLock();
+ private final Lock shutdownMembersReadLock = shutdownMembersLock.readLock();
+ private final Lock shutdownMembersWriteLock =
shutdownMembersLock.writeLock();
/**
* per bug 39552, keep a list of members that have been shunned and for
which a message is
* printed. Contents of this list are cleared at the same time they are
removed from
* {@link #shunnedMembers}.
*
- * Accesses to this list needs to be under the read or write lock of {@link
#latestViewLock}
+ * Writing to this list needs to be under {@link #latestViewWriteLock}
*/
- private final HashSet<ID> shunnedAndWarnedMembers = new HashSet<>();
+ private final Set<ID> shunnedAndWarnedMembers =
+ Collections.newSetFromMap(new ConcurrentHashMap<>());
/**
* The identities and birth-times of others that we have allowed into
membership at the
* distributed system level, but have not yet appeared in a view.
@@ -295,7 +305,7 @@ public class GMSMembership<ID extends MemberIdentifier>
implements Membership<ID
* {@link #surpriseMemberTimeout} milliseconds have passed, a view
containing the member has not
* arrived, the member is removed from membership and member-left
notification is performed.
* <p>
- * > Accesses to this list needs to be under the read or write lock of
{@link #latestViewLock}
+ * > Writing to this list needs to be under {@link #latestViewWriteLock}
*
* @see System#currentTimeMillis()
*/
@@ -359,7 +369,7 @@ public class GMSMembership<ID extends MemberIdentifier>
implements Membership<ID
/**
* Analyze a given view object, generate events as appropriate
*/
- public void processView(MembershipView<ID> newView) {
+ public void processView(final MembershipView<ID> newView) {
// Sanity check...
if (logger.isDebugEnabled()) {
StringBuilder msg = new StringBuilder(200);
@@ -388,7 +398,7 @@ public class GMSMembership<ID extends MemberIdentifier>
implements Membership<ID
// update the view to reflect our changes, so that
// callbacks will see the new (updated) view.
- MembershipView<ID> newlatestView = new MembershipView<>(newView,
newView.getViewId());
+ MembershipView<ID> newlatestView = newView;
// look for additions
for (int i = 0; i < newView.getMembers().size(); i++) { // additions
@@ -488,7 +498,7 @@ public class GMSMembership<ID extends MemberIdentifier>
implements Membership<ID
"not seen in membership view in " + this.surpriseMemberTimeout +
"ms");
} else {
if (!newlatestView.contains(entry.getKey())) {
- newlatestView.add(entry.getKey());
+ newlatestView =
newlatestView.createNewViewWithMember(entry.getKey());
}
}
}
@@ -505,7 +515,6 @@ public class GMSMembership<ID extends MemberIdentifier>
implements Membership<ID
}
// the view is complete - let's install it
- newlatestView.makeUnmodifiable();
latestView = newlatestView;
listener.viewInstalled(latestView);
} finally {
@@ -535,21 +544,14 @@ public class GMSMembership<ID extends MemberIdentifier>
implements Membership<ID
}
}
- public boolean isCleanupTimerStarted() {
- return this.cleanupTimer != null;
- }
-
/**
* the timer used to perform periodic tasks
- *
- * Concurrency: protected by {@link #latestViewLock} ReentrantReadWriteLock
*/
private ScheduledExecutorService cleanupTimer;
private Services<ID> services;
-
/**
* Joins the distributed system
*
@@ -567,9 +569,7 @@ public class GMSMembership<ID extends MemberIdentifier>
implements Membership<ID
// connect
services.getJoinLeave().join();
- MembershipView<ID> initialView =
createGeodeView(services.getJoinLeave().getView());
- latestView = new MembershipView<>(initialView,
initialView.getViewId());
- latestView.makeUnmodifiable();
+ latestView = createGeodeView(services.getJoinLeave().getView());
listener.viewInstalled(latestView);
} finally {
this.isJoining = false;
@@ -580,28 +580,9 @@ public class GMSMembership<ID extends MemberIdentifier>
implements Membership<ID
}
private MembershipView<ID> createGeodeView(GMSMembershipView<ID> view) {
- MembershipView<ID> result =
- createGeodeView(view.getCreator(), view.getViewId(), view.getMembers(),
- view.getShutdownMembers(),
- view.getCrashedMembers());
- result.makeUnmodifiable();
- return result;
- }
-
- private MembershipView<ID> createGeodeView(ID gmsCreator, int viewId,
- List<ID> gmsMembers,
- Set<ID> gmsShutdowns, Set<ID> gmsCrashes) {
- ID geodeCreator = gmsCreator;
- List<ID> geodeMembers = new ArrayList<>(gmsMembers.size());
- for (ID member : gmsMembers) {
- geodeMembers.add(member);
- }
- Set<ID> geodeShutdownMembers =
- gmsMemberCollectionToIDSet(gmsShutdowns);
- Set<ID> geodeCrashedMembers =
- gmsMemberCollectionToIDSet(gmsCrashes);
- return new MembershipView<>(geodeCreator, viewId, geodeMembers,
geodeShutdownMembers,
- geodeCrashedMembers);
+ return new MembershipView<>(view.getCreator(), view.getViewId(),
view.getMembers(),
+ view.getShutdownMembers(),
+ view.getCrashedMembers());
}
private Set<ID> gmsMemberCollectionToIDSet(
@@ -664,7 +645,8 @@ public class GMSMembership<ID extends MemberIdentifier>
implements Membership<ID
}
/**
- * Remove a member. {@link #latestViewLock} must be held before this method
is called. If member
+ * Remove a member. {@link #latestViewWriteLock} must be held before this
method is called. If
+ * member
* is not already shunned, the uplevel event handler is invoked.
*/
private void removeWithViewLock(ID dm, boolean crashed, String reason) {
@@ -677,13 +659,22 @@ public class GMSMembership<ID extends MemberIdentifier>
implements Membership<ID
return; // Explicit deletion, no upcall.
}
- if (!shutdownMembers.containsKey(dm)) {
+ if (!isMemberShuttingDown(dm)) {
// if we've received a shutdown message then DistributionManager will
already have
// notified listeners
listener.memberDeparted(dm, crashed, reason);
}
}
+ private boolean isMemberShuttingDown(ID dm) {
+ shutdownMembersReadLock.lock();
+ try {
+ return shutdownMembers.contains(dm);
+ } finally {
+ shutdownMembersReadLock.unlock();
+ }
+ }
+
/**
* Process a surprise connect event, or place it on the startup queue.
*
@@ -714,8 +705,8 @@ public class GMSMembership<ID extends MemberIdentifier>
implements Membership<ID
* Logic for handling a direct connection event (message received from a
member not in the view).
* Does not employ the startup queue.
* <p>
- * Must be called with {@link #latestViewLock} held. Waits until there is a
stable view. If the
- * member has already been added, simply returns; else adds the member.
+ * Waits until there is a stable view. If the member has already been added,
simply returns;
+ * else adds the member.
*
* @param dm the member joining
*/
@@ -786,10 +777,7 @@ public class GMSMembership<ID extends MemberIdentifier>
implements Membership<ID
// should ensure it is not chosen as an elder.
// This will get corrected when the member finally shows up in the
// view.
- MembershipView<ID> newMembers = new MembershipView<>(latestView,
latestView.getViewId());
- newMembers.add(member);
- newMembers.makeUnmodifiable();
- latestView = newMembers;
+ latestView = latestView.createNewViewWithMember(member);
}
} finally {
latestViewWriteLock.unlock();
@@ -858,18 +846,13 @@ public class GMSMembership<ID extends MemberIdentifier>
implements Membership<ID
@Override
public void warnShun(ID m) {
- latestViewWriteLock.lock();
- try {
- if (!shunnedMembers.containsKey(m)) {
- return; // not shunned
- }
- if (shunnedAndWarnedMembers.contains(m)) {
- return; // already warned
- }
- shunnedAndWarnedMembers.add(m);
- } finally {
- latestViewWriteLock.unlock();
+ if (!shunnedMembers.containsKey(m)) {
+ return; // not shunned
+ }
+ if (shunnedAndWarnedMembers.contains(m)) {
+ return; // already warned
}
+ shunnedAndWarnedMembers.add(m);
// issue warning outside of sync since it may cause messaging and we don't
// want to hold the view lock while doing that
logger.warn("Membership: disregarding shunned member <{}>", m);
@@ -890,26 +873,21 @@ public class GMSMembership<ID extends MemberIdentifier>
implements Membership<ID
// If this member is shunned or new, grab the latestViewWriteLock: update
the appropriate data
// structure.
if (isShunnedOrNew(m)) {
- latestViewWriteLock.lock();
- try {
- if (isShunned(m)) {
- if (msg instanceof StopShunningMarker) {
- endShun(m);
- } else {
- // fix for bug 41538 - sick alert listener causes deadlock
- // due to view latestViewReadWriteLock being held during messaging
- shunned = true;
- }
+ if (isShunned(m)) {
+ if (msg instanceof StopShunningMarker) {
+ endShun(m);
+ } else {
+ // fix for bug 41538 - sick alert listener causes deadlock
+ // due to view latestViewReadWriteLock being held during messaging
+ shunned = true;
}
+ }
- if (!shunned) {
- // If it's a new sender, wait our turn, generate the event
- if (isNew(m)) {
- shunned = !addSurpriseMember(m);
- }
+ if (!shunned) {
+ // If it's a new sender, wait our turn, generate the event
+ if (isNew(m)) {
+ shunned = !addSurpriseMember(m);
}
- } finally {
- latestViewWriteLock.unlock();
}
}
@@ -965,22 +943,16 @@ public class GMSMembership<ID extends MemberIdentifier>
implements Membership<ID
return;
}
}
- latestViewWriteLock.lock();
- try {
- if (!processingEvents) {
- synchronized (startupLock) {
- if (!startupMessagesDrained) {
- startupMessages.add(new StartupEvent<>(viewArg));
- return;
- }
+ if (!processingEvents) {
+ synchronized (startupLock) {
+ if (!startupMessagesDrained) {
+ startupMessages.add(new StartupEvent<>(viewArg));
+ return;
}
}
-
- viewExecutor.submit(() -> processView(viewArg));
-
- } finally {
- latestViewWriteLock.unlock();
}
+
+ viewExecutor.submit(() -> processView(viewArg));
}
private ID gmsMemberToDMember(ID gmsMember) {
@@ -993,23 +965,17 @@ public class GMSMembership<ID extends MemberIdentifier>
implements Membership<ID
* @param suspectInfo the suspectee and suspector
*/
protected void handleOrDeferSuspect(SuspectMember<ID> suspectInfo) {
- latestViewWriteLock.lock();
- try {
- if (!processingEvents) {
- return;
- }
- ID suspect = gmsMemberToDMember(suspectInfo.suspectedMember);
- ID who = gmsMemberToDMember(suspectInfo.whoSuspected);
- this.suspectedMembers.put(suspect,
Long.valueOf(System.currentTimeMillis()));
- listener.memberSuspect(suspect, who, suspectInfo.reason);
- } finally {
- latestViewWriteLock.unlock();
+ if (!processingEvents) {
+ return;
}
+ ID suspect = gmsMemberToDMember(suspectInfo.suspectedMember);
+ ID who = gmsMemberToDMember(suspectInfo.whoSuspected);
+ this.suspectedMembers.put(suspect, System.currentTimeMillis());
+ listener.memberSuspect(suspect, who, suspectInfo.reason);
}
/**
- * Process a potential direct connect. Does not use the startup queue. It
grabs the
- * {@link #latestViewLock} and then processes the event.
+ * Process a potential direct connect. Does not use the startup queue.
* <p>
* It is a <em>potential</em> event, because we don't know until we've
grabbed a stable view if
* this is really a new member.
@@ -1145,10 +1111,7 @@ public class GMSMembership<ID extends MemberIdentifier>
implements Membership<ID
*/
@Override
public MembershipView<ID> getView() {
- // Grab the latest view under a mutex...
- MembershipView<ID> v = latestView;
- MembershipView<ID> result = new MembershipView<>(v, v.getViewId());
- return result;
+ return latestView;
}
public boolean isJoining() {
@@ -1162,20 +1125,13 @@ public class GMSMembership<ID extends MemberIdentifier>
implements Membership<ID
*/
@Override
public ID getCoordinator() {
- latestViewReadLock.lock();
- try {
- return latestView == null ? null : latestView.getCoordinator();
- } finally {
- latestViewReadLock.unlock();
- }
+ final MembershipView<ID> view = latestView;
+ return view == null ? null : view.getCoordinator();
}
@Override
public boolean memberExists(ID m) {
- latestViewReadLock.lock();
- MembershipView<ID> v = latestView;
- latestViewReadLock.unlock();
- return v.contains(m);
+ return latestView.contains(m);
}
/**
@@ -1231,23 +1187,24 @@ public class GMSMembership<ID extends MemberIdentifier>
implements Membership<ID
if (logger.isDebugEnabled()) {
logger.debug("Membership: recording shutdown status of {}", id);
}
- synchronized (this.shutdownMembers) {
- this.shutdownMembers.put(id, id);
- services.getHealthMonitor()
- .memberShutdown(id, reason);
+ shutdownMembersWriteLock.lock();
+ try {
+ this.shutdownMembers.add(id);
services.getJoinLeave().memberShutdown(id, reason);
+ } finally {
+ shutdownMembersWriteLock.unlock();
}
}
@Override
public Set<ID> getMembersNotShuttingDown() {
- latestViewReadLock.lock();
+ shutdownMembersReadLock.lock();
try {
- return latestView.getMembers().stream().filter(id ->
!shutdownMembers.containsKey(id))
+ return latestView.getMembers().stream().filter(id ->
!shutdownMembers.contains(id))
.collect(
Collectors.toSet());
} finally {
- latestViewReadLock.unlock();
+ shutdownMembersReadLock.unlock();
}
}
@@ -1335,7 +1292,7 @@ public class GMSMembership<ID extends MemberIdentifier>
implements Membership<ID
@Override
public void suspectMember(ID mbr, String reason) {
- if (!this.shutdownInProgress && !this.shutdownMembers.containsKey(mbr)) {
+ if (!this.shutdownInProgress && !isMemberShuttingDown(mbr)) {
verifyMember(mbr, reason);
}
}
@@ -1360,13 +1317,8 @@ public class GMSMembership<ID extends MemberIdentifier>
implements Membership<ID
@Override
public ID[] getAllMembers(final ID[] arrayType) {
- latestViewReadLock.lock();
- try {
- List<ID> keySet = latestView.getMembers();
- return keySet.toArray(arrayType);
- } finally {
- latestViewReadLock.unlock();
- }
+ final List<ID> keySet = latestView.getMembers();
+ return keySet.toArray(arrayType);
}
@Override
@@ -1448,6 +1400,10 @@ public class GMSMembership<ID extends MemberIdentifier>
implements Membership<ID
}
}
+ /**
+ * This method holds the {@link #latestViewWriteLock} to protect
+ * {@link #shutdownInProgress} from modification while {@link #processView}
is in progress.
+ */
@Override
public void setShutdown() {
latestViewWriteLock.lock();
@@ -1457,8 +1413,6 @@ public class GMSMembership<ID extends MemberIdentifier>
implements Membership<ID
/**
* Clean up and create consistent new view with member removed. No uplevel
events are generated.
- *
- * Must be called with the {@link #latestViewLock} held.
*/
private void destroyMember(final ID member, final String reason) {
@@ -1466,17 +1420,13 @@ public class GMSMembership<ID extends MemberIdentifier>
implements Membership<ID
latestViewWriteLock.lock();
try {
if (latestView.contains(member)) {
- MembershipView<ID> newView = new MembershipView<>(latestView,
latestView.getViewId());
- newView.remove(member);
- newView.makeUnmodifiable();
- latestView = newView;
+ latestView = latestView.createNewViewWithoutMember(member);
}
+ surpriseMembers.remove(member);
} finally {
latestViewWriteLock.unlock();
}
- surpriseMembers.remove(member);
-
// Trickiness: there is a minor recursion
// with addShunnedMembers, since it will
// attempt to destroy really really old members. Performing the check
@@ -1496,8 +1446,6 @@ public class GMSMembership<ID extends MemberIdentifier>
implements Membership<ID
* This also checks the time the given member was shunned, and has
the side effect of
* removing the member from the list if it was shunned too far in the
past.
*
- * Concurrency: protected by {@link #latestViewLock}
ReentrantReadWriteLock
- *
* @return true if the given member is a zombie
*/
@Override
@@ -1506,21 +1454,16 @@ public class GMSMembership<ID extends MemberIdentifier>
implements Membership<ID
return false;
}
- latestViewWriteLock.lock();
- try {
- // Make sure that the entry isn't stale...
- long shunTime = shunnedMembers.get(m).longValue();
- long now = System.currentTimeMillis();
- if (shunTime + SHUNNED_SUNSET * 1000L > now) {
- return true;
- }
-
- // Oh, it _is_ stale. Remove it while we're here.
- endShun(m);
- return false;
- } finally {
- latestViewWriteLock.unlock();
+ // Make sure that the entry isn't stale...
+ long shunTime = shunnedMembers.get(m).longValue();
+ long now = System.currentTimeMillis();
+ if (shunTime + SHUNNED_SUNSET * 1000L > now) {
+ return true;
}
+
+ // Oh, it _is_ stale. Remove it while we're here.
+ endShun(m);
+ return false;
}
private boolean isShunnedOrNew(final ID m) {
@@ -1545,7 +1488,7 @@ public class GMSMembership<ID extends MemberIdentifier>
implements Membership<ID
* <p>
* Like isShunned, this method holds the view lock while executing
*
- * Concurrency: protected by {@link #latestViewLock} ReentrantReadWriteLock
+ * Concurrency: protected by {@link #latestViewReadLock}
*
* @param m the member in question
* @return true if the given member is a surprise member
@@ -1555,8 +1498,8 @@ public class GMSMembership<ID extends MemberIdentifier>
implements Membership<ID
latestViewReadLock.lock();
try {
if (surpriseMembers.containsKey(m)) {
- long birthTime = surpriseMembers.get(m).longValue();
- long now = System.currentTimeMillis();
+ final long birthTime = surpriseMembers.get(m);
+ final long now = System.currentTimeMillis();
return (birthTime >= (now - this.surpriseMemberTimeout));
}
return false;
@@ -1602,7 +1545,6 @@ public class GMSMembership<ID extends MemberIdentifier>
implements Membership<ID
* Add the given member to the shunned list. Also, purge any shunned members
that are really
* really old.
* <p>
- * Must be called with {@link #latestViewLock} held and the view stable.
*
* @param m the member to add
*/
@@ -1687,25 +1629,22 @@ public class GMSMembership<ID extends MemberIdentifier>
implements Membership<ID
return shutdownInProgress;
}
- // TODO GEODE-1752 rewrite this to get rid of the latches, which are
currently a memory leak
@Override
public boolean waitForNewMember(ID remoteId) {
boolean foundRemoteId = false;
CountDownLatch currentLatch = null;
- // ARB: preconditions
- // remoteId != null
+ // latestViewWriteLock protects memberLatch from modification while
processView is in progress
latestViewWriteLock.lock();
try {
- if (latestView == null) {
- // Not sure how this would happen, but see bug 38460.
- // No view?? Not found!
- } else if (latestView.contains(remoteId)) {
- // ARB: check if remoteId is already in membership view.
- // If not, then create a latch if needed and wait for the latch to
open.
- foundRemoteId = true;
- } else if ((currentLatch = this.memberLatch.get(remoteId)) == null) {
- currentLatch = new CountDownLatch(1);
- this.memberLatch.put(remoteId, currentLatch);
+ if (latestView != null) {
+ if (latestView.contains(remoteId)) {
+ // check if remoteId is already in membership view.
+ // If not, then create a latch if needed and wait for the latch to
open.
+ foundRemoteId = true;
+ } else if ((currentLatch = this.memberLatch.get(remoteId)) == null) {
+ currentLatch = new CountDownLatch(1);
+ this.memberLatch.put(remoteId, currentLatch);
+ }
}
} finally {
latestViewWriteLock.unlock();
@@ -1716,18 +1655,13 @@ public class GMSMembership<ID extends MemberIdentifier>
implements Membership<ID
if (currentLatch != null
&& currentLatch.await(membershipCheckTimeout,
TimeUnit.MILLISECONDS)) {
foundRemoteId = true;
- // @todo
- // ARB: remove latch from memberLatch map if this is the last thread
waiting on latch.
}
} catch (InterruptedException ex) {
- // ARB: latch attempt was interrupted.
+ // latch attempt was interrupted.
Thread.currentThread().interrupt();
logger.warn("The membership check was terminated with an exception.");
}
}
-
- // ARB: postconditions
- // (foundRemoteId == true) ==> (currentLatch is non-null ==> currentLatch
is open)
return foundRemoteId;
}
@@ -1889,29 +1823,18 @@ public class GMSMembership<ID extends MemberIdentifier>
implements Membership<ID
/* Service interface */
@Override
- public void started() throws MemberStartupException {
+ public void started() {
startCleanupTimer();
}
/* Service interface */
@Override
public void stop() {
- // [bruce] Do not null out the channel w/o adding appropriate
synchronization
-
logger.debug("Membership closing");
if (lifecycleListener.disconnect(null)) {
-
if (address != null) {
- // Make sure that channel information is consistent
- // Probably not important in this particular case, but just
- // to be consistent...
- latestViewWriteLock.lock();
- try {
- destroyMember(address, "orderly shutdown");
- } finally {
- latestViewWriteLock.unlock();
- }
+ destroyMember(address, "orderly shutdown");
}
}
