Fix connection closing before initialization 26/45526/5
authorTomas Slusny <tomas.slusny@pantheon.sk>
Tue, 13 Sep 2016 06:26:15 +0000 (08:26 +0200)
committerTomas Slusny <tomas.slusny@pantheon.sk>
Mon, 19 Sep 2016 08:39:39 +0000 (10:39 +0200)
Check if connection is fully initialized when closing
objects created during initialization phase to prevent
NPEs, what can cause incorrect close of connection
and later program unstability.

Also, increase timeout to propagate role to 10 seconds
in case of slower machine.

Resolves: bug 5271
See also: bug 6672

Change-Id: I0f009edf5fe3b382e2bfee64f72036ba599ccc5d
Signed-off-by: Tomas Slusny <tomas.slusny@pantheon.tech>
openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/device/DeviceContextImpl.java
openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/lifecycle/LifecycleServiceImpl.java
openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/role/RoleContextImpl.java
openflowplugin-impl/src/test/java/org/opendaylight/openflowplugin/impl/lifecycle/LifecycleServiceImplTest.java

index 7c61eac96a6c92cbabb433e1bc84b8e3e0adfcbf..1f032d03cc0b5bc9a01f3af1b6be0b218f56f27a 100644 (file)
@@ -18,14 +18,12 @@ import java.math.BigInteger;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.Objects;
 import java.util.concurrent.ExecutionException;
 import javax.annotation.Nonnull;
 import org.opendaylight.controller.md.sal.binding.api.DataBroker;
 import org.opendaylight.controller.md.sal.binding.api.NotificationPublishService;
 import org.opendaylight.controller.md.sal.binding.api.ReadOnlyTransaction;
 import org.opendaylight.controller.md.sal.common.api.data.LogicalDatastoreType;
-import org.opendaylight.controller.md.sal.common.api.data.TransactionChainClosedException;
 import org.opendaylight.mdsal.singleton.common.api.ServiceGroupIdentifier;
 import org.opendaylight.openflowjava.protocol.api.connection.ConnectionAdapter;
 import org.opendaylight.openflowjava.protocol.api.keys.MessageTypeKey;
@@ -100,9 +98,6 @@ import org.opendaylight.yangtools.yang.binding.KeyedInstanceIdentifier;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-/**
- *
- */
 public class DeviceContextImpl implements DeviceContext, ExtensionConverterProviderKeeper{
 
     private static final Logger LOG = LoggerFactory.getLogger(DeviceContextImpl.class);
@@ -180,7 +175,9 @@ public class DeviceContextImpl implements DeviceContext, ExtensionConverterProvi
 
     @Override
     public void initialSubmitTransaction() {
-        transactionChainManager.initialSubmitWriteTransaction();
+        if (initialized) {
+            transactionChainManager.initialSubmitWriteTransaction();
+        }
     }
 
     @Override
@@ -215,7 +212,7 @@ public class DeviceContextImpl implements DeviceContext, ExtensionConverterProvi
     public <T extends DataObject> void writeToTransaction(final LogicalDatastoreType store,
                                                           final InstanceIdentifier<T> path,
                                                           final T data){
-        if (Objects.nonNull(transactionChainManager)) {
+        if (initialized) {
             transactionChainManager.writeToTransaction(store, path, data, false);
         }
     }
@@ -224,21 +221,21 @@ public class DeviceContextImpl implements DeviceContext, ExtensionConverterProvi
     public <T extends DataObject> void writeToTransactionWithParentsSlow(final LogicalDatastoreType store,
                                                                          final InstanceIdentifier<T> path,
                                                                          final T data){
-        if (Objects.nonNull(transactionChainManager)) {
+        if (initialized) {
             transactionChainManager.writeToTransaction(store, path, data, true);
         }
     }
 
     @Override
     public <T extends DataObject> void addDeleteToTxChain(final LogicalDatastoreType store, final InstanceIdentifier<T> path) {
-        if (Objects.nonNull(transactionChainManager)) {
+        if (initialized) {
             transactionChainManager.addDeleteOperationTotTxChain(store, path);
         }
     }
 
     @Override
     public boolean submitTransaction() {
-        return Objects.nonNull(transactionChainManager) && transactionChainManager.submitWriteTransaction();
+        return initialized && transactionChainManager.submitWriteTransaction();
     }
 
     @Override
@@ -505,26 +502,29 @@ public class DeviceContextImpl implements DeviceContext, ExtensionConverterProvi
             LOG.debug("ConnectionCtx for Node {} is in RIP state.", getDeviceInfo().getLOGValue());
             return;
         }
-        /* Terminate Auxiliary Connection */
+
+        // Terminate Auxiliary Connection
         for (final ConnectionContext connectionContext : auxiliaryConnectionContexts.values()) {
             LOG.debug("Closing auxiliary connection {}", connectionContext.getNodeId());
             connectionContext.closeConnection(false);
         }
-        /* Terminate Primary Connection */
+
+        // Terminate Primary Connection
         getPrimaryConnectionContext().closeConnection(true);
-        /* Close all Group Registry */
-        deviceGroupRegistry.close();
-        deviceFlowRegistry.close();
-        deviceMeterRegistry.close();
+
+        // Close all datastore registries
+        if (initialized) {
+            deviceGroupRegistry.close();
+            deviceFlowRegistry.close();
+            deviceMeterRegistry.close();
+        }
     }
 
     @Override
     public ListenableFuture<Void> shuttingDownDataStoreTransactions() {
-        ListenableFuture<Void> future = Futures.immediateFuture(null);
-        if (Objects.nonNull(this.transactionChainManager)) {
-            future = this.transactionChainManager.shuttingDown();
-        }
-        return future;
+        return initialized
+                ? this.transactionChainManager.shuttingDown()
+                : Futures.immediateFuture(null);
     }
 
     @VisibleForTesting
@@ -549,11 +549,9 @@ public class DeviceContextImpl implements DeviceContext, ExtensionConverterProvi
 
     @Override
     public ListenableFuture<Void> stopClusterServices(boolean deviceDisconnected) {
-        ListenableFuture<Void> future = Futures.immediateFuture(null);
-        if (Objects.nonNull(this.transactionChainManager)) {
-            future = this.transactionChainManager.deactivateTransactionManager();
-        }
-        return future;
+        return initialized
+                ? this.transactionChainManager.deactivateTransactionManager()
+                : Futures.immediateFuture(null);
     }
 
     @Override
@@ -568,7 +566,7 @@ public class DeviceContextImpl implements DeviceContext, ExtensionConverterProvi
 
     @Override
     public void putLifecycleServiceIntoTxChainManager(final LifecycleService lifecycleService){
-        if (Objects.nonNull(this.transactionChainManager)) {
+        if (initialized) {
             this.transactionChainManager.setLifecycleService(lifecycleService);
         }
     }
index 4ada870f2dbca5b17de3a4271645813720232570..948b5a1d2e069c27c3e8273e25a93f3207fc2768 100644 (file)
@@ -79,6 +79,7 @@ public class LifecycleServiceImpl implements LifecycleService {
     @Override
     public void close() throws Exception {
         if (registration != null) {
+            LOG.info("Unregistering clustering MASTER services for node {}", this.deviceContext.getDeviceInfo().getLOGValue());
             registration.close();
             registration = null;
         }
@@ -86,6 +87,8 @@ public class LifecycleServiceImpl implements LifecycleService {
 
     @Override
     public void registerService(final ClusterSingletonServiceProvider singletonServiceProvider) {
+        LOG.info("Registering clustering MASTER services for node {}", this.deviceContext.getDeviceInfo().getLOGValue());
+
         //lifecycle service -> device context -> statistics context -> rpc context -> role context -> lifecycle service
         this.clusterInitializationPhaseHandler = deviceContext;
         this.deviceContext.setLifecycleInitializationPhaseHandler(this.statContext);
index b636c9fb3021aa379c9651fca999c8ff84be70ca..58d8ab212837a24175546514c31646b083498ce0 100644 (file)
@@ -49,6 +49,9 @@ class RoleContextImpl implements RoleContext {
 
     private static final Logger LOG = LoggerFactory.getLogger(RoleContextImpl.class);
 
+    // Timeout in seconds after what we will give up on propagating role
+    private static final int SET_ROLE_TIMEOUT = 10;
+
     private SalRoleService salRoleService = null;
     private final HashedWheelTimer hashedWheelTimer;
     private final DeviceInfo deviceInfo;
@@ -162,7 +165,7 @@ class RoleContextImpl implements RoleContext {
                     setRoleOutputFuture.cancel(true);
                 }
             };
-            hashedWheelTimer.newTimeout(timerTask, 5, TimeUnit.SECONDS);
+            hashedWheelTimer.newTimeout(timerTask, SET_ROLE_TIMEOUT, TimeUnit.SECONDS);
         } else {
             LOG.info("Device: {} with version: {} does not support role", deviceInfo.getLOGValue(), deviceInfo.getVersion());
             return Futures.immediateFuture(null);
index 21f3f9460b838c67d883b7fefc6e57ca11ea84f9..96f4dcecdf92d386fbe9693b396b8e142d2ceac1 100644 (file)
@@ -54,12 +54,6 @@ public class LifecycleServiceImplTest {
 
     @Before
     public void setUp() {
-        lifecycleService = new LifecycleServiceImpl();
-        lifecycleService.setDeviceContext(deviceContext);
-        lifecycleService.setRpcContext(rpcContext);
-        lifecycleService.setRoleContext(roleContext);
-        lifecycleService.setStatContext(statContext);
-        lifecycleService.registerService(clusterSingletonServiceProvider);
         Mockito.when(deviceContext.getDeviceInfo()).thenReturn(deviceInfo);
         Mockito.when(deviceContext.getPrimaryConnectionContext()).thenReturn(connectionContext);
         Mockito.when(deviceContext.getDeviceFlowRegistry()).thenReturn(deviceFlowRegistry);
@@ -67,6 +61,13 @@ public class LifecycleServiceImplTest {
         Mockito.when(deviceFlowRegistry.fill()).thenReturn(Futures.immediateFuture(null));
         Mockito.when(connectionContext.getConnectionState()).thenReturn(ConnectionContext.CONNECTION_STATE.WORKING);
         Mockito.when(deviceInfo.getLOGValue()).thenReturn(TEST_NODE);
+
+        lifecycleService = new LifecycleServiceImpl();
+        lifecycleService.setDeviceContext(deviceContext);
+        lifecycleService.setRpcContext(rpcContext);
+        lifecycleService.setRoleContext(roleContext);
+        lifecycleService.setStatContext(statContext);
+        lifecycleService.registerService(clusterSingletonServiceProvider);
     }
 
     @Test