[GitHub] [geode] bschuchardt commented on a change in pull request #6037: GEODE-7245: Remove most uses of latestViewReadLock in GMSMembership

[GitBox <gi...@apache.org>]

bschuchardt commented on a change in pull request #6037:
URL: https://github.com/apache/geode/pull/6037#discussion_r579399721



##########
File path: geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMembership.java
##########
@@ -273,6 +273,9 @@ boolean isDistributionMessage() {
   /**
    * 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.
+   *
+   * Accesses to this map should be synchronized on the map to avoid concurrent
+   * modification exceptions
    */
   private final Map<ID, Object> shutdownMembers = new BoundedLinkedHashMap<>();

Review comment:
       I'm wondering if this could be made a synchronized collection with Collections.synchronizedMap().  Then we could get rid of the external synchronization.

##########
File path: geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMembership.java
##########
@@ -677,10 +658,12 @@ private void removeWithViewLock(ID dm, boolean crashed, String reason) {
       return; // Explicit deletion, no upcall.
     }
 
-    if (!shutdownMembers.containsKey(dm)) {
-      // if we've received a shutdown message then DistributionManager will already have
-      // notified listeners
-      listener.memberDeparted(dm, crashed, reason);
+    synchronized (shutdownMembers) {
+      if (!shutdownMembers.containsKey(dm)) {
+        // if we've received a shutdown message then DistributionManager will already have
+        // notified listeners
+        listener.memberDeparted(dm, crashed, reason);

Review comment:
       Is it necessary to hold the lock on shutdownMembers while invoking a listener?  The point of this check is to avoid invoking memberDeparted if a shutdown message from **dm** has already been processed.  In that case we've already invoked memberDeparted on the listener.

##########
File path: geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMembership.java
##########
@@ -1524,15 +1487,9 @@ public boolean isShunned(ID m) {
   }
 
   private boolean isShunnedOrNew(final ID m) {
-    latestViewReadLock.lock();
-    try {
-      return shunnedMembers.containsKey(m) || isNew(m);
-    } finally { // synchronized
-      latestViewReadLock.unlock();
-    }
+    return shunnedMembers.containsKey(m) || isNew(m);
   }
 
-  // must be invoked under view read or write lock
   private boolean isNew(final ID m) {
     return !latestView.contains(m) && !surpriseMembers.containsKey(m);

Review comment:
       I think the read-lock is still needed here.  While the "shunned" state of the member is disconnected from the other two checks, surpriseMembers is modified while forming a new membership view and we want these two checks to be atomic.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org