From 5fffad88c5940f04001bee1ac4190d47f5fea34b Mon Sep 17 00:00:00 2001 From: Tomas Slusny Date: Tue, 13 Sep 2016 08:26:15 +0200 Subject: [PATCH] Fix connection closing before initialization 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 Signed-off-by: Shuva Kar --- .../impl/device/DeviceContextImpl.java | 53 +++++++++---------- .../impl/lifecycle/LifecycleServiceImpl.java | 3 ++ .../impl/role/RoleContextImpl.java | 5 +- .../lifecycle/LifecycleServiceImplTest.java | 13 ++--- 4 files changed, 40 insertions(+), 34 deletions(-) diff --git a/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/device/DeviceContextImpl.java b/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/device/DeviceContextImpl.java index 6eb240a174..c7d60e6f9a 100644 --- a/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/device/DeviceContextImpl.java +++ b/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/device/DeviceContextImpl.java @@ -18,7 +18,6 @@ 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 java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; @@ -102,9 +101,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); @@ -184,7 +180,9 @@ public class DeviceContextImpl implements DeviceContext, ExtensionConverterProvi @Override public void initialSubmitTransaction() { - transactionChainManager.initialSubmitWriteTransaction(); + if (initialized) { + transactionChainManager.initialSubmitWriteTransaction(); + } } @Override @@ -219,7 +217,7 @@ public class DeviceContextImpl implements DeviceContext, ExtensionConverterProvi public void writeToTransaction(final LogicalDatastoreType store, final InstanceIdentifier path, final T data){ - if (Objects.nonNull(transactionChainManager)) { + if (initialized) { transactionChainManager.writeToTransaction(store, path, data, false); } } @@ -228,21 +226,21 @@ public class DeviceContextImpl implements DeviceContext, ExtensionConverterProvi public void writeToTransactionWithParentsSlow(final LogicalDatastoreType store, final InstanceIdentifier path, final T data){ - if (Objects.nonNull(transactionChainManager)) { + if (initialized) { transactionChainManager.writeToTransaction(store, path, data, true); } } @Override - public void addDeleteToTxChain(final LogicalDatastoreType store, final InstanceIdentifier path) { - if (Objects.nonNull(transactionChainManager)) { + public void addDeleteToTxChain(final LogicalDatastoreType store, final InstanceIdentifier path) throws TransactionChainClosedException { + if (initialized) { transactionChainManager.addDeleteOperationTotTxChain(store, path); } } @Override public boolean submitTransaction() { - return Objects.nonNull(transactionChainManager) && transactionChainManager.submitWriteTransaction(); + return initialized && transactionChainManager.submitWriteTransaction(); } @Override @@ -557,26 +555,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 shuttingDownDataStoreTransactions() { - ListenableFuture future = Futures.immediateFuture(null); - if (Objects.nonNull(this.transactionChainManager)) { - future = this.transactionChainManager.shuttingDown(); - } - return future; + return initialized + ? this.transactionChainManager.shuttingDown() + : Futures.immediateFuture(null); } @VisibleForTesting @@ -601,11 +602,9 @@ public class DeviceContextImpl implements DeviceContext, ExtensionConverterProvi @Override public ListenableFuture stopClusterServices(boolean deviceDisconnected) { - ListenableFuture future = Futures.immediateFuture(null); - if (Objects.nonNull(this.transactionChainManager)) { - future = this.transactionChainManager.deactivateTransactionManager(); - } - return future; + return initialized + ? this.transactionChainManager.deactivateTransactionManager() + : Futures.immediateFuture(null); } @Override @@ -620,7 +619,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); } } diff --git a/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/lifecycle/LifecycleServiceImpl.java b/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/lifecycle/LifecycleServiceImpl.java index abbe25802d..7b6cbe58ee 100644 --- a/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/lifecycle/LifecycleServiceImpl.java +++ b/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/lifecycle/LifecycleServiceImpl.java @@ -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); diff --git a/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/role/RoleContextImpl.java b/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/role/RoleContextImpl.java index b636c9fb30..58d8ab2128 100644 --- a/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/role/RoleContextImpl.java +++ b/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/role/RoleContextImpl.java @@ -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); diff --git a/openflowplugin-impl/src/test/java/org/opendaylight/openflowplugin/impl/lifecycle/LifecycleServiceImplTest.java b/openflowplugin-impl/src/test/java/org/opendaylight/openflowplugin/impl/lifecycle/LifecycleServiceImplTest.java index 21f3f9460b..96f4dcecdf 100644 --- a/openflowplugin-impl/src/test/java/org/opendaylight/openflowplugin/impl/lifecycle/LifecycleServiceImplTest.java +++ b/openflowplugin-impl/src/test/java/org/opendaylight/openflowplugin/impl/lifecycle/LifecycleServiceImplTest.java @@ -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 -- 2.36.6