BUG-8156 Fix PCEP topology registration

It is possible that when a PCEP session is being dropped and being
unregistered from ServerSessionManager, the thread gets switched
thus the topology unregistering cannot finish.

If such situation happens, PCEP session from the same PCEP peer will
not be able to established until after DEADTIMER_EXPIRED, as the
topology instance was not removed correctly.

Change-Id: I647ecfbbb35ef6805d563753b7ebd87edfc350fe
Signed-off-by: Kevin Wang <kevixw@gmail.com>

diff --git a/pcep/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/AbstractTopologySessionListener.java b/pcep/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/AbstractTopologySessionListener.java
index b44bdf77489f6a72c047001ce4c3bb10f6347799..06b8910e6c90a866711a1211682ddeb234603434 100755 (executable)
--- a/pcep/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/AbstractTopologySessionListener.java
+++ b/pcep/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/AbstractTopologySessionListener.java
@@ -26,6 +26,7 @@ import java.util.concurrent.TimeUnit;
 import javax.annotation.concurrent.GuardedBy;
 import org.opendaylight.controller.config.yang.pcep.topology.provider.ListenerStateRuntimeMXBean;
 import org.opendaylight.controller.config.yang.pcep.topology.provider.ListenerStateRuntimeRegistration;
+import org.opendaylight.controller.config.yang.pcep.topology.provider.PCEPTopologyProviderRuntimeRegistration;
 import org.opendaylight.controller.config.yang.pcep.topology.provider.PeerCapabilities;
 import org.opendaylight.controller.config.yang.pcep.topology.provider.ReplyTime;
 import org.opendaylight.controller.config.yang.pcep.topology.provider.SessionState;
@@ -125,6 +126,7 @@ public abstract class AbstractTopologySessionListener<S, L> implements PCEPSessi
     private boolean triggeredResyncInProcess;
 
     private ListenerStateRuntimeRegistration registration;
+    @GuardedBy("this")
     private final SessionListenerState listenerState;
 
     protected AbstractTopologySessionListener(final ServerSessionManager serverSessionManager) {
@@ -147,11 +149,13 @@ public abstract class AbstractTopologySessionListener<S, L> implements PCEPSessi
         final TopologyNodeState state = this.serverSessionManager.takeNodeState(peerAddress, this, isLspDbRetreived());
         // takeNodeState(..) may fail when the server session manager is being restarted due to configuration change
         if (state == null) {
+            LOG.error("Unable to fetch topology node state for PCEP session. Closing session {}", session);
             this.onSessionDown(session, new RuntimeException("Unable to fetch topology node state for PCEP session with " + session.getRemoteAddress()));
             return;
         }
 
         if (this.session != null || this.nodeState != null) {
+            LOG.error("PCEP session is already up. Closing session {}", session);
             this.onSessionDown(session, new IllegalStateException("Session is already up with " + session.getRemoteAddress()));
             return;
         }
@@ -177,12 +181,13 @@ public abstract class AbstractTopologySessionListener<S, L> implements PCEPSessi
             pccBuilder.setReportedLsp(initialNodeState.getAugmentation(Node1.class).getPathComputationClient().getReportedLsp());
         }
         writeNode(pccBuilder, state, topologyAugment);
-        this.listenerState.init(session);
-        this.registration = this.serverSessionManager.registerRuntimeRootRegistration(this);
+        register();
         if (this.registration == null) {
+            LOG.error("PCEP session fails to register. Closing session {}", session);
             this.onSessionDown(session, new RuntimeException("PCEP Session with " + session.getRemoteAddress() + " fails to register."));
             return;
         }
+        this.listenerState.init(session);
         LOG.info("Session with {} attached to topology node {}", session.getRemoteAddress(), state.getNodeId());
     }
 
@@ -288,10 +293,14 @@ public abstract class AbstractTopologySessionListener<S, L> implements PCEPSessi
 
     @Override
     public final synchronized void onMessage(final PCEPSession session, final Message message) {
+        if (this.nodeState == null) {
+            LOG.warn("Topology node state is null. Unhandled message {} on session {}", message, session);
+            return;
+        }
         final MessageContext ctx = new MessageContext(this.nodeState.beginTransaction());
 
         if (onMessage(ctx, message)) {
-            LOG.info("Unhandled message {} on session {}", message, session);
+            LOG.warn("Unhandled message {} on session {}", message, session);
             //cancel not supported, submit empty transaction
             ctx.trans.submit();
             return;
@@ -321,16 +330,25 @@ public abstract class AbstractTopologySessionListener<S, L> implements PCEPSessi
         }
     }
 
-    private synchronized void unregister() {
+    private final synchronized void unregister() {
         if (this.registration != null) {
             this.registration.close();
-            LOG.trace("PCEP session {} unregistered successfully.", this.session);
+            LOG.trace("PCEP session {} is unregistered successfully.", this.session);
             this.registration = null;
         } else {
             LOG.trace("PCEP session {} was not registered.", this.session);
         }
     }
 
+    private final synchronized void register() {
+        Preconditions.checkState(this.registration == null);
+        final PCEPTopologyProviderRuntimeRegistration runtimeReg = this.serverSessionManager.getRuntimeRootRegistration();
+        if (runtimeReg != null) {
+            this.registration = runtimeReg.register(this);
+            LOG.trace("PCEP session {} is successfully registered.", this.session);
+        }
+    }
+
     protected final synchronized PCEPRequest removeRequest(final S id) {
         final PCEPRequest ret = this.requests.remove(id);
         if (ret != null) {
@@ -610,7 +628,7 @@ public abstract class AbstractTopologySessionListener<S, L> implements PCEPSessi
         return false;
     }
 
-    protected SessionListenerState getSessionListenerState() {
+    protected synchronized SessionListenerState getSessionListenerState() {
         return this.listenerState;
     }
 
@@ -635,12 +653,12 @@ public abstract class AbstractTopologySessionListener<S, L> implements PCEPSessi
     }
 
     @Override
-    public ReplyTime getReplyTime() {
+    public synchronized ReplyTime getReplyTime() {
         return this.listenerState.getReplyTime();
     }
 
     @Override
-    public PeerCapabilities getPeerCapabilities() {
+    public synchronized PeerCapabilities getPeerCapabilities() {
         return this.listenerState.getPeerCapabilities();
     }
 

diff --git a/pcep/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/ServerSessionManager.java b/pcep/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/ServerSessionManager.java
index 34d109907c408c8d82a256460d8c5bc3c0b9d485..228e33f9f8500b79408d8996dba340faca415a3a 100755 (executable)
--- a/pcep/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/ServerSessionManager.java
+++ b/pcep/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/ServerSessionManager.java
@@ -21,8 +21,6 @@ import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicReference;
 import javax.annotation.concurrent.GuardedBy;
-import org.opendaylight.controller.config.yang.pcep.topology.provider.ListenerStateRuntimeMXBean;
-import org.opendaylight.controller.config.yang.pcep.topology.provider.ListenerStateRuntimeRegistration;
 import org.opendaylight.controller.config.yang.pcep.topology.provider.PCEPTopologyProviderRuntimeMXBean;
 import org.opendaylight.controller.config.yang.pcep.topology.provider.PCEPTopologyProviderRuntimeRegistration;
 import org.opendaylight.controller.config.yang.pcep.topology.provider.PCEPTopologyProviderRuntimeRegistrator;
@@ -232,17 +230,10 @@ final class ServerSessionManager implements PCEPSessionListenerFactory, Topology
         }
     }
 
-    ListenerStateRuntimeRegistration registerRuntimeRootRegistration(final ListenerStateRuntimeMXBean bean) {
-        final PCEPTopologyProviderRuntimeRegistration runtimeReg = this.runtimeRootRegistration.get();
-        if (runtimeReg != null) {
-            final ListenerStateRuntimeRegistration reg = runtimeReg.register(bean);
-            LOG.trace("Bean {} is successfully registered.", bean.getPeerId());
-            return reg;
-        }
-        return null;
+    PCEPTopologyProviderRuntimeRegistration getRuntimeRootRegistration() {
+        return this.runtimeRootRegistration.get();
     }
 
-    @Override
     public void setPeerSpecificProposal(final InetSocketAddress address, final TlvsBuilder openBuilder) {
         Preconditions.checkNotNull(address);
         this.peerProposal.setPeerProposal(createNodeId(address.getAddress()), openBuilder);