Replaced synchronized with locks (https://issues.redhat.com/browse/JGRP-2863)

belaban · belaban · commit b6c27218e4f0 · 2025-02-11T14:17:02.000+01:00
diff --git a/src/org/jgroups/Membership.java b/src/org/jgroups/Membership.java
@@ -89,7 +89,6 @@ public Membership add(Collection<Address> v) {
         return this;
     }
 
-
    /**
     * Removes a member from the membership. If this member doesn't exist, no action will be
     * performed on the existing membership
@@ -104,7 +103,6 @@ public Membership remove(Address old_member) {
         return this;
     }
 
-
    /**
     * Removes all the members contained in v from this membership
     * @param v a list of all the members to be removed
@@ -127,7 +125,6 @@ public Membership retainAll(Collection<Address> v) {
         return this;
     }
 
-
     /**
      * Removes all the members from this membership
      */
@@ -257,6 +254,12 @@ public boolean isCoord(Address mbr) {
         }
     }
 
+    public Address nextCoord() {
+        synchronized(members) {
+            return members.size() > 1? members.get(1) : null;
+        }
+    }
+
     /**
      * Returns the members to the left of mbr
      * @param mbr The member whose neighbor to the left should be returned
diff --git a/src/org/jgroups/protocols/pbcast/GMS.java b/src/org/jgroups/protocols/pbcast/GMS.java
@@ -110,7 +110,7 @@ public class GMS extends Protocol implements DiagnosticsHandler.ProbeHandler {
     protected int                       num_views;
     protected BoundedList<String>       prev_views;  // History of the last N views
     protected GmsImpl                   impl;
-    protected final Lock                impl_lock=new ReentrantLock(); // synchronizes event entry into impl
+    protected final Lock                lock=new ReentrantLock();
     protected final Map<String,GmsImpl> impls=new HashMap<>(3);
 
     protected Merger                    merger; // handles merges
@@ -309,14 +309,14 @@ public List<Integer> providedDownServices() {
     }
 
     public void setImpl(GmsImpl new_impl) {
-        impl_lock.lock();
+        lock.lock();
         try {
             if(impl == new_impl)
                 return;
             impl=new_impl;
         }
         finally {
-            impl_lock.unlock();
+            lock.unlock();
         }
     }
 
@@ -433,7 +433,8 @@ public void cancelMerge() {
      * {@code suspected_mbrs} removed and {@code joiners} added.
      */
     public View getNextView(Collection<Address> joiners, Collection<Address> leavers, Collection<Address> suspected_mbrs) {
-        synchronized(members) {
+        lock.lock();
+        try {
             ViewId view_id=view != null? view.getViewId() : null;
             if(view_id == null) {
                 log.error(Util.getMessage("ViewidIsNull"));
@@ -461,6 +462,9 @@ public View getNextView(Collection<Address> joiners, Collection<Address> leavers
                 suspected_mbrs.stream().filter(addr -> !leaving.contains(addr)).forEach(leaving::add);
             return v;
         }
+        finally {
+            lock.unlock();
+        }
     }
 
     /** Computes the regular membership */
@@ -602,42 +606,43 @@ public void installView(View new_view) {
      * Sets the new view and sends a VIEW_CHANGE event up and down the stack. If the view is a MergeView (subclass
      * of View), then digest will be non-null and has to be set before installing the view.
      */
-    public synchronized void installView(View new_view, Digest digest) {
-        ViewId vid=new_view.getViewId();
+    public void installView(View new_view, Digest digest) {
+        Event         view_event;
+        ViewId        vid=new_view.getViewId();
         List<Address> mbrs=new_view.getMembers();
-        ltime=Math.max(vid.getId(), ltime);  // compute the logical time, regardless of whether the view is accepted
+        boolean       am_i_coord;
+        lock.lock();
+        try {
+            ltime=Math.max(vid.getId(), ltime);  // compute the logical time, regardless of whether the view is accepted
 
-        // Discards view with id lower than or equal to our own. Will be installed without check if it is the first view
-        if(view != null && vid.compareToIDs(view.getViewId()) <= 0)
-            return;
+            // Discards view with id lower than or equal to our own. Will be installed without check if it is the first view
+            if(view != null && vid.compareToIDs(view.getViewId()) <= 0)
+                return;
 
-        /* Check for self-inclusion: if I'm not part of the new membership, I just discard it.
+            /* Check for self-inclusion: if I'm not part of the new membership, I just discard it.
            This ensures that messages sent in view V1 are only received by members of V1 */
-        if(!mbrs.contains(local_addr)) {
-            if(log_view_warnings)
-                log.warn("%s: not member of view %s; discarding it", local_addr, new_view.getViewId());
-            return;
-        }
+            if(!mbrs.contains(local_addr)) {
+                if(log_view_warnings)
+                    log.warn("%s: not member of view %s; discarding it", local_addr, new_view.getViewId());
+                return;
+            }
 
-        if(digest != null) {
-            if(new_view instanceof MergeView)
-                mergeDigest(digest);
-            else
-                setDigest(digest);
-        }
+            if(digest != null) {
+                if(new_view instanceof MergeView)
+                    mergeDigest(digest);
+                else
+                    setDigest(digest);
+            }
 
-        if(log.isDebugEnabled()) {
-            Address[][] diff=View.diff(view, new_view);
-            log.debug("%s: installing view %s %s", local_addr, new_view,
-                      print_view_details? View.printDiff(diff) : "");
-        }
+            if(log.isDebugEnabled()) {
+                Address[][] diff=View.diff(view, new_view);
+                log.debug("%s: installing view %s %s", local_addr, new_view,
+                          print_view_details? View.printDiff(diff) : "");
+            }
 
-        Event view_event;
-        boolean was_coord, is_coord;
-        synchronized(members) {
-            was_coord=view != null && Objects.equals(local_addr, view.getCoord());
+            boolean was_coord=view != null && Objects.equals(local_addr, view.getCoord());
             view=new_view;
-            is_coord=Objects.equals(local_addr, view.getCoord());
+            boolean is_coord=Objects.equals(local_addr, view.getCoord());
             view_event=new Event(Event.VIEW_CHANGE, new_view);
 
             // Set the membership. Take into account joining members
@@ -647,12 +652,8 @@ public synchronized void installView(View new_view, Digest digest) {
                 joining.removeAll(mbrs);  // remove all members in mbrs from joining
                 // remove all elements from 'leaving' that are not in 'mbrs'
                 leaving.retainAll(mbrs);
-
-                tmp_members.add(joining);    // add members that haven't yet shown up in the membership
-                tmp_members.remove(leaving); // remove members that haven't yet been removed from the membership
+                tmp_members.add(joining).remove(leaving);
                 suspected_mbrs.retainAll(mbrs);
-
-                // add to prev_members
                 mbrs.stream().filter(addr -> !prev_members.contains(addr)).forEach(addr -> prev_members.add(addr));
             }
 
@@ -664,6 +665,15 @@ public synchronized void installView(View new_view, Digest digest) {
                 if(was_coord || impl instanceof ClientGmsImpl)
                     becomeParticipant();
             }
+            ack_collector.retainAll(new_view.getMembers());
+            if(stats) {
+                num_views++;
+                prev_views.add(Util.utcNow() + ": " + new_view);
+            }
+            am_i_coord=Objects.equals(local_addr, new_view.getCoord());
+        }
+        finally {
+            lock.unlock();
         }
 
         // - Changed order of passing view up and down (https://issues.redhat.com/browse/JGRP-347)
@@ -672,45 +682,31 @@ public synchronized void installView(View new_view, Digest digest) {
         down_prot.down(view_event); // needed e.g. by failure detector or UDP
         up_prot.up(view_event);
 
-        List<Address> tmp_mbrs=new_view.getMembers();
-        ack_collector.retainAll(tmp_mbrs);
-
-        if(new_view instanceof MergeView) {
-            // Everybody except the merge leader cancels the merge, otherwise - if UNICAST3.loopback is true - we'd
-            // interrupt our own thread which will fail code that later sends a message before returning!
-            // Note that the merge leader does cancel the merge later, after having installed the MergeView
-            // (in Merger.handleMergeView() in the finally clause)
-            if(!Objects.equals(local_addr, new_view.getCoord()))
-                merger.forceCancelMerge();
-        }
-
-        if(stats) {
-            num_views++;
-            prev_views.add(Util.utcNow() + ": " + new_view);
-        }
+        // Everybody except the merge leader cancels the merge, otherwise - if UNICAST3.loopback is true - we'd
+        // interrupt our own thread which will fail code that later sends a message before returning!
+        // Note that the merge leader does cancel the merge later, after having installed the MergeView
+        // (in Merger.handleMergeView() in the finally clause)
+        if(new_view instanceof MergeView && !am_i_coord)
+            merger.forceCancelMerge();
     }
 
     protected Address getCoord() {
-        impl_lock.lock();
+        lock.lock();
         try {
             return isCoord()? determineNextCoordinator() : determineCoordinator();
         }
         finally {
-            impl_lock.unlock();
+            lock.unlock();
         }
     }
 
     protected Address determineCoordinator() {
-        synchronized(members) {
-            return members.getFirst();
-        }
+        return members.getFirst();
     }
 
     /** Returns the second-in-line */
     protected Address determineNextCoordinator() {
-        synchronized(members) {
-            return members.size() > 1? members.elementAt(1) : null;
-        }
+        return members.nextCoord();
     }
 
     protected static View createDeltaView(final View current_view, final View next_view) {
@@ -720,36 +716,35 @@ protected static View createDeltaView(final View current_view, final View next_v
         return new DeltaView(next_view_id, current_view_id, diff[1], diff[0]);
     }
 
-
     /** Checks whether the potential_new_coord would be the new coordinator (2nd in line) */
     protected boolean wouldBeNewCoordinator(Address potential_new_coord) {
         if(potential_new_coord == null) return false;
-        synchronized(members) {
+        lock.lock();
+        try {
             if(members.size() < 2) return false;
             Address new_coord=members.elementAt(1);  // member at 2nd place
             return Objects.equals(new_coord, potential_new_coord);
         }
+        finally {
+            lock.unlock();
+        }
     }
 
-
     /** Send down a SET_DIGEST event */
     public void setDigest(Digest d) {
         down_prot.down(new Event(Event.SET_DIGEST, d));
     }
 
-
     /** Send down a MERGE_DIGEST event */
     public void mergeDigest(Digest d) {
         down_prot.down(new Event(Event.MERGE_DIGEST,d));
     }
 
-
     /** Grabs the current digest from NAKACK{2} */
     public Digest getDigest() {
         return (Digest)down_prot.down(Event.GET_DIGEST_EVT);
     }
 
-
     public Object up(Event evt) {
         switch(evt.getType()) {
             case Event.SUSPECT:
diff --git a/tests/junit-functional/org/jgroups/tests/MembershipTest.java b/tests/junit-functional/org/jgroups/tests/MembershipTest.java
@@ -34,7 +34,6 @@ void setUp() {
         m1=new Membership();
     }
 
-
     public void testConstructor() {
         v1=Arrays.asList(a1, a2, a3);
         m2=new Membership(v1);
@@ -44,7 +43,6 @@ public void testConstructor() {
         assert m2.contains(a3);
     }
 
-
     public void testClone() {
         v1=Arrays.asList(a1, a2, a3);
         m2=new Membership(v1);
@@ -56,8 +54,6 @@ public void testClone() {
         assert m2.contains(a2);
     }
 
-
-
     public void testCopy() {
         v1=Arrays.asList(a1, a2, a3);
         m2=new Membership(v1);
@@ -69,8 +65,6 @@ public void testCopy() {
         assert m2.contains(a2);
     }
 
-
-
     public void testAdd() {
         m1.add(a1, a2, a3);
         assert m1.size() == 2;
@@ -79,8 +73,6 @@ public void testAdd() {
         assert m1.contains(a3);
     }
 
-
-
     public void testAddVector() {
         v1=Arrays.asList(a1, a2, a3);
         m1.add(v1);
@@ -89,7 +81,6 @@ public void testAddVector() {
         assert m1.contains(a2);
     }
 
-
     public void testAddVectorDupl() {
         v1=Arrays.asList(a1, a2, a3, a4, a5);
         m1.add(a5, a1).add(v1);
@@ -100,13 +91,11 @@ public void testAddVectorDupl() {
         assert m1.contains(a5);
     }
 
-
     public void testRemove() {
         m1.add(a1, a2, a3, a4, a5).remove(a2);
         assert m1.size() == 3;
     }
 
-
     public void testGetMembers() {
         testAdd();
         List<Address> v=m1.getMembers();
@@ -138,6 +127,16 @@ public void testIsCoord() {
         assert !coord;
     }
 
+    public void testNextCoord() {
+        m1.add(a1,a2);
+        assert m1.isCoord(a1);
+        Address tmp=m1.nextCoord();
+        assert a2.equals(tmp);
+        m1.remove(a2);
+        tmp=m1.nextCoord();
+        assert tmp == null;
+    }
+
     public void testGetNext() {
         Address next=m1.getNext(null);
         assert next == null;
