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"); } }