Fix BGPPeer internal locking

onRouteRefreshMessage() is not synchronized and manipulates
adjRibOutListenerSet, which is a thread-safety hazard.

Fix by holding a lock for the duration needed, as well as
use a single remove() operation instead of get/remove combo.

Finally lower private method synchronization and replace it
with @GuardedBy annotations, so that it is understood those
methods are only ever called with the lock already held.

Change-Id: Idd509a345dfc3afa800e7456fb6d3bdbd1f367e7
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>

diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/BGPPeer.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/BGPPeer.java
index b2fa31e8aa61059ac159bf8e1c4313ac453496b5..c739f8beb80b668d15ff51540f0643a018d26915 100644 (file)
--- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/BGPPeer.java
+++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/BGPPeer.java
@@ -260,13 +260,14 @@ public class BGPPeer extends AbstractPeer implements BGPSessionListener {
         final Class<? extends SubsequentAddressFamily> rrSafi = message.getSafi();
 
         final TablesKey key = new TablesKey(rrAfi, rrSafi);
-        final AdjRibOutListener listener = this.adjRibOutListenerSet.get(key);
-        if (listener != null) {
-            listener.close();
-            this.adjRibOutListenerSet.remove(key);
-            createAdjRibOutListener(key, listener.isMpSupported());
-        } else {
-            LOG.info("Ignoring RouteRefresh message. Afi/Safi is not supported: {}, {}.", rrAfi, rrSafi);
+        synchronized (this) {
+            final AdjRibOutListener listener = this.adjRibOutListenerSet.remove(key);
+            if (listener != null) {
+                listener.close();
+                createAdjRibOutListener(key, listener.isMpSupported());
+            } else {
+                LOG.info("Ignoring RouteRefresh message. Afi/Safi is not supported: {}, {}.", rrAfi, rrSafi);
+            }
         }
     }
 
@@ -330,7 +331,8 @@ public class BGPPeer extends AbstractPeer implements BGPSessionListener {
         }
     }
 
-    private synchronized void handleGracefulEndOfRib() {
+    @GuardedBy("this")
+    private void handleGracefulEndOfRib() {
         if (isLocalRestarting()) {
             if (this.missingEOT.isEmpty()) {
                 createEffRibInWriter();
@@ -446,7 +448,8 @@ public class BGPPeer extends AbstractPeer implements BGPSessionListener {
     }
 
     //try to add a support for old-school BGP-4, if peer did not advertise IPv4-Unicast MP capability
-    private synchronized void addBgp4Support() {
+    @GuardedBy("this")
+    private void addBgp4Support() {
         if (!this.tables.contains(IPV4_UCAST_TABLE_KEY)) {
             final HashSet<TablesKey> newSet = new HashSet<>(this.tables);
             newSet.add(IPV4_UCAST_TABLE_KEY);
@@ -455,8 +458,8 @@ public class BGPPeer extends AbstractPeer implements BGPSessionListener {
         }
     }
 
-    private synchronized void createAdjRibOutListener(final TablesKey key,
-                                                      final boolean mpSupport) {
+    @GuardedBy("this")
+    private void createAdjRibOutListener(final TablesKey key, final boolean mpSupport) {
         final RIBSupport<?, ?, ?, ?> ribSupport = this.rib.getRibSupportContext().getRIBSupport(key);
 
         // not particularly nice