BUG-4491: Race condition under PCEP Topology Provider

Race condition under PCEP Topology Provider when
instace is reconfigured, since closeInstance under CSS
is nonbloking, it can race the delete with the writing
of the new instance when restarting.

Change-Id: If79bf34f77223bc56424a57e4e600fdb17ddd154
Signed-off-by: Claudio D. Gasparini <claudio.gasparini@pantheon.tech>

diff --git a/pcep/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/config/PCEPTopologyDeployerImpl.java b/pcep/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/config/PCEPTopologyDeployerImpl.java
index 4d08045ee5151e65ccb618bed37f1321fd381727..050a8dae4fda41f93f15560d6da968e7d85fc046 100644 (file)
--- a/pcep/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/config/PCEPTopologyDeployerImpl.java
+++ b/pcep/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/config/PCEPTopologyDeployerImpl.java
@@ -12,6 +12,9 @@ import com.google.common.base.Preconditions;
 import java.net.InetSocketAddress;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
 import javax.annotation.concurrent.GuardedBy;
 import org.opendaylight.bgpcep.programming.spi.InstructionScheduler;
 import org.opendaylight.controller.config.yang.pcep.topology.provider.PCEPTopologyProviderRuntimeRegistrator;
@@ -23,6 +26,7 @@ import org.slf4j.LoggerFactory;
 
 public class PCEPTopologyDeployerImpl implements PCEPTopologyDeployer, AutoCloseable {
     private static final Logger LOG = LoggerFactory.getLogger(PCEPTopologyDeployerImpl.class);
+    private static final long TIMEOUT_NS = TimeUnit.SECONDS.toNanos(5);
 
     @GuardedBy("this")
     private final Map<TopologyId, PCEPTopologyProviderBean> pcepTopologyServices = new HashMap<>();
@@ -53,12 +57,19 @@ public class PCEPTopologyDeployerImpl implements PCEPTopologyDeployer, AutoClose
     public synchronized void removeTopologyProvider(final TopologyId topologyID) {
         final PCEPTopologyProviderBean service = this.pcepTopologyServices.remove(topologyID);
         if (service != null) {
+            try {
+                service.closeServiceInstance().get(TIMEOUT_NS, TimeUnit.NANOSECONDS);
+            } catch (final InterruptedException | ExecutionException | TimeoutException e) {
+                LOG.error("Failed to close Topology Provider {}.", topologyID.getValue(), e);
+            }
+
             service.close();
         }
     }
 
     @Override
     public synchronized void close() throws Exception {
+        this.pcepTopologyServices.values().forEach(PCEPTopologyProviderBean::closeServiceInstance);
         this.pcepTopologyServices.values().forEach(PCEPTopologyProviderBean::close);
     }
 }

diff --git a/pcep/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/config/PCEPTopologyProviderBean.java b/pcep/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/config/PCEPTopologyProviderBean.java
index 5c26e6fffb6d68b3d50db1bc43e0c9142c8b8f2d..b9caa8aca2f545799107cd8166e2184141b75a93 100644 (file)
--- a/pcep/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/config/PCEPTopologyProviderBean.java
+++ b/pcep/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/config/PCEPTopologyProviderBean.java
@@ -42,6 +42,7 @@ public final class PCEPTopologyProviderBean implements PCEPTopologyProviderDepen
     private final RpcProviderRegistry rpcProviderRegistry;
     private final BundleContext bundleContext;
     private final ClusterSingletonServiceProvider cssp;
+    @GuardedBy("this")
     private PCEPTopologyProviderBeanCSS pcepTopoProviderCSS;
 
     public PCEPTopologyProviderBean(final ClusterSingletonServiceProvider cssp, final BundleContext bundleContext,
@@ -61,14 +62,22 @@ public final class PCEPTopologyProviderBean implements PCEPTopologyProviderDepen
         }
     }
 
+    synchronized ListenableFuture<Void> closeServiceInstance() {
+        if (this.pcepTopoProviderCSS != null) {
+            return this.pcepTopoProviderCSS.closeServiceInstance();
+        }
+        return Futures.immediateFuture(null);
+    }
+
     @Override
-    public void close() {
+    public synchronized void close() {
         if (this.pcepTopoProviderCSS != null) {
             this.pcepTopoProviderCSS.close();
+            this.pcepTopoProviderCSS = null;
         }
     }
 
-    public void start(final PCEPTopologyConfigDependencies configDependencies) {
+    synchronized void start(final PCEPTopologyConfigDependencies configDependencies) {
         Preconditions.checkState(this.pcepTopoProviderCSS == null,
             "Previous instance %s was not closed.", this);
         try {
@@ -120,7 +129,7 @@ public final class PCEPTopologyProviderBean implements PCEPTopologyProviderDepen
 
         @Override
         public synchronized void instantiateServiceInstance() {
-            LOG.info("Topology Provider Singleton Service {} instantiated", getIdentifier().getValue());
+            LOG.info("PCEP Topology Provider Singleton Service {} instantiated", getIdentifier().getValue());
             if (this.pcepTopoProvider != null) {
                 this.pcepTopoProvider.instantiateServiceInstance();
                 this.serviceInstantiated = true;
@@ -129,7 +138,7 @@ public final class PCEPTopologyProviderBean implements PCEPTopologyProviderDepen
 
         @Override
         public synchronized ListenableFuture<Void> closeServiceInstance() {
-            LOG.info("Close Topology Provider Singleton Service {}", getIdentifier().getValue());
+            LOG.info("Close PCEP Topology Provider Singleton Service {}", getIdentifier().getValue());
             if (this.pcepTopoProvider != null && this.serviceInstantiated) {
                 this.serviceInstantiated = false;
                 return this.pcepTopoProvider.closeServiceInstance();
@@ -144,7 +153,7 @@ public final class PCEPTopologyProviderBean implements PCEPTopologyProviderDepen
         }
 
         @Override
-        public void close() {
+        public synchronized void close() {
             if (this.cssRegistration != null) {
                 try {
                     this.cssRegistration.close();