BUG-7901: fix unsynchronized transaction access
[openflowplugin.git] / openflowplugin-impl / src / main / java / org / opendaylight / openflowplugin / impl / device / TransactionChainManager.java
index 00dd6828dc74ccc2386e20c2964c8d6f50bbedce..aded9b26d9ee3d76f6222f9ae172934ddf872885 100644 (file)
@@ -14,6 +14,9 @@ import com.google.common.util.concurrent.CheckedFuture;
 import com.google.common.util.concurrent.FutureCallback;
 import com.google.common.util.concurrent.Futures;
 import com.google.common.util.concurrent.ListenableFuture;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.concurrent.CancellationException;
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
 import javax.annotation.concurrent.GuardedBy;
@@ -27,13 +30,9 @@ import org.opendaylight.controller.md.sal.common.api.data.TransactionChainClosed
 import org.opendaylight.controller.md.sal.common.api.data.TransactionChainListener;
 import org.opendaylight.controller.md.sal.common.api.data.TransactionCommitFailedException;
 import org.opendaylight.openflowplugin.api.openflow.device.DeviceInfo;
-import org.opendaylight.openflowplugin.api.openflow.lifecycle.LifecycleConductor;
-import org.opendaylight.yang.gen.v1.urn.opendaylight.inventory.rev130819.NodeId;
-import org.opendaylight.yang.gen.v1.urn.opendaylight.inventory.rev130819.nodes.Node;
-import org.opendaylight.yang.gen.v1.urn.opendaylight.inventory.rev130819.nodes.NodeKey;
+import org.opendaylight.openflowplugin.api.openflow.lifecycle.LifecycleService;
 import org.opendaylight.yangtools.yang.binding.DataObject;
 import org.opendaylight.yangtools.yang.binding.InstanceIdentifier;
-import org.opendaylight.yangtools.yang.binding.KeyedInstanceIdentifier;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -45,20 +44,16 @@ import org.slf4j.LoggerFactory;
  * a {@link TransactionChainListener} and provide package protected methods for writeToTransaction
  * method (wrapped {@link WriteTransaction#put(LogicalDatastoreType, InstanceIdentifier, DataObject)})
  * and submitTransaction method (wrapped {@link WriteTransaction#submit()})
- *
- * @author <a href="mailto:vdemcak@cisco.com">Vaclav Demcak</a>
- *         </p>
- *         Created: Apr 2, 2015
  */
 class TransactionChainManager implements TransactionChainListener, AutoCloseable {
 
     private static final Logger LOG = LoggerFactory.getLogger(TransactionChainManager.class);
+    private static final String CANNOT_WRITE_INTO_TRANSACTION = "Cannot write into transaction.";
 
     private final Object txLock = new Object();
-    private final KeyedInstanceIdentifier<Node, NodeKey> nodeII;
-    private final DeviceInfo deviceInfo;
     private final DataBroker dataBroker;
-    private final LifecycleConductor conductor;
+    private final String nodeId;
+    private LifecycleService lifecycleService;
 
     @GuardedBy("txLock")
     private WriteTransaction wTx;
@@ -69,37 +64,27 @@ class TransactionChainManager implements TransactionChainListener, AutoCloseable
     @GuardedBy("txLock")
     private ListenableFuture<Void> lastSubmittedFuture;
 
-    private boolean initCommit;
-
-    public TransactionChainManagerStatus getTransactionChainManagerStatus() {
-        return transactionChainManagerStatus;
-    }
+    private volatile boolean initCommit;
 
     @GuardedBy("txLock")
     private TransactionChainManagerStatus transactionChainManagerStatus = TransactionChainManagerStatus.SLEEPING;
 
     TransactionChainManager(@Nonnull final DataBroker dataBroker,
-                            @Nonnull final DeviceInfo deviceInfo,
-                            @Nonnull final LifecycleConductor conductor) {
-        this.dataBroker = Preconditions.checkNotNull(dataBroker);
-        this.conductor = Preconditions.checkNotNull(conductor);
-        this.deviceInfo = deviceInfo;
-        this.nodeII = deviceInfo.getNodeInstanceIdentifier();
-        this.transactionChainManagerStatus = TransactionChainManagerStatus.SLEEPING;
-        lastSubmittedFuture = Futures.immediateFuture(null);
-        LOG.debug("created txChainManager for {}", this.nodeII);
-    }
-
-    private NodeId nodeId() {
-        return nodeII.getKey().getId();
+                            @Nonnull final DeviceInfo deviceInfo) {
+        this.dataBroker = dataBroker;
+        this.nodeId = deviceInfo.getNodeInstanceIdentifier().getKey().getId().getValue();
+        this.lastSubmittedFuture = Futures.immediateFuture(null);
     }
 
     @GuardedBy("txLock")
     private void createTxChain() {
-        if (txChainFactory != null) {
-            txChainFactory.close();
-        }
+        BindingTransactionChain txChainFactoryTemp = txChainFactory;
         txChainFactory = dataBroker.createTransactionChain(TransactionChainManager.this);
+        Optional.ofNullable(txChainFactoryTemp).ifPresent(TransactionChain::close);
+    }
+
+    public void setLifecycleService(final LifecycleService lifecycleService) {
+        this.lifecycleService = lifecycleService;
     }
 
     void initialSubmitWriteTransaction() {
@@ -113,18 +98,17 @@ class TransactionChainManager implements TransactionChainListener, AutoCloseable
      * transactions. Call this method for MASTER role only.
      */
     void activateTransactionManager() {
-        LOG.trace("activateTransactionManager for node {} transaction submit is set to {}", nodeId(), submitIsEnabled);
+        if (LOG.isDebugEnabled()) {
+            LOG.debug("activateTransactionManager for node {} transaction submit is set to {}", this.nodeId, submitIsEnabled);
+        }
         synchronized (txLock) {
-            if (TransactionChainManagerStatus.SLEEPING.equals(transactionChainManagerStatus)) {
-                LOG.debug("Transaction Factory create {}", nodeId());
+            if (TransactionChainManagerStatus.SLEEPING == transactionChainManagerStatus) {
                 Preconditions.checkState(txChainFactory == null, "TxChainFactory survive last close.");
                 Preconditions.checkState(wTx == null, "We have some unexpected WriteTransaction.");
                 this.transactionChainManagerStatus = TransactionChainManagerStatus.WORKING;
                 this.submitIsEnabled = false;
                 this.initCommit = true;
                 createTxChain();
-            } else {
-                LOG.debug("Transaction is active {}", nodeId());
             }
         }
     }
@@ -136,25 +120,24 @@ class TransactionChainManager implements TransactionChainListener, AutoCloseable
      * @return Future
      */
     ListenableFuture<Void> deactivateTransactionManager() {
+        if (LOG.isDebugEnabled()) {
+            LOG.debug("deactivateTransactionManager for node {}", this.nodeId);
+        }
         final ListenableFuture<Void> future;
         synchronized (txLock) {
-            if (TransactionChainManagerStatus.WORKING.equals(transactionChainManagerStatus)) {
-                LOG.debug("Submitting all transactions if we were in status WORKING for Node {}", nodeId());
+            if (TransactionChainManagerStatus.WORKING == transactionChainManagerStatus) {
                 transactionChainManagerStatus = TransactionChainManagerStatus.SLEEPING;
                 future = txChainShuttingDown();
                 Preconditions.checkState(wTx == null, "We have some unexpected WriteTransaction.");
-                LOG.debug("Transaction Factory deactivate for Node {}", nodeId());
                 Futures.addCallback(future, new FutureCallback<Void>() {
                     @Override
                     public void onSuccess(final Void result) {
-                        txChainFactory.close();
-                        txChainFactory = null;
+                        removeTxChainFactory();
                     }
 
                     @Override
                     public void onFailure(final Throwable t) {
-                        txChainFactory.close();
-                        txChainFactory = null;
+                        removeTxChainFactory();
                     }
                 });
             } else {
@@ -165,78 +148,95 @@ class TransactionChainManager implements TransactionChainListener, AutoCloseable
         return future;
     }
 
+    private void removeTxChainFactory() {
+        Optional.ofNullable(txChainFactory).ifPresent(TransactionChain::close);
+        txChainFactory = null;
+    }
+
     boolean submitWriteTransaction() {
         synchronized (txLock) {
             if (!submitIsEnabled) {
-                LOG.trace("transaction not committed - submit block issued");
+                if (LOG.isTraceEnabled()) {
+                    LOG.trace("transaction not committed - submit block issued");
+                }
                 return false;
             }
-            if (wTx == null) {
-                LOG.trace("nothing to commit - submit returns true");
+            if (Objects.isNull(wTx)) {
+                if (LOG.isTraceEnabled()) {
+                    LOG.trace("nothing to commit - submit returns true");
+                }
                 return true;
             }
-            Preconditions.checkState(TransactionChainManagerStatus.WORKING.equals(transactionChainManagerStatus),
-                    "we have here Uncompleted Transaction for node {} and we are not MASTER", nodeII);
+            Preconditions.checkState(TransactionChainManagerStatus.WORKING == transactionChainManagerStatus,
+                    "we have here Uncompleted Transaction for node {} and we are not MASTER", this.nodeId);
             final CheckedFuture<Void, TransactionCommitFailedException> submitFuture = wTx.submit();
+            lastSubmittedFuture = submitFuture;
+            wTx = null;
+
             Futures.addCallback(submitFuture, new FutureCallback<Void>() {
                 @Override
                 public void onSuccess(final Void result) {
-                    if (initCommit) {
-                        initCommit = false;
-                    }
+                    initCommit = false;
                 }
 
                 @Override
                 public void onFailure(final Throwable t) {
                     if (t instanceof TransactionCommitFailedException) {
-                        LOG.error("Transaction commit failed. {}", t);
+                        LOG.error("Transaction commit failed. ", t);
                     } else {
-                        LOG.error("Exception during transaction submitting. {}", t);
+                        if (t instanceof CancellationException) {
+                            LOG.warn("Submit task was canceled");
+                            LOG.trace("Submit exception: ", t);
+                        } else {
+                            LOG.error("Exception during transaction submitting. ", t);
+                        }
                     }
                     if (initCommit) {
-                        LOG.error("Initial commit failed. {}", t);
-                        conductor.closeConnection(deviceInfo);
+                        Optional.ofNullable(lifecycleService).ifPresent(LifecycleService::closeConnection);
                     }
                 }
             });
-            lastSubmittedFuture = submitFuture;
-            wTx = null;
         }
         return true;
     }
 
     <T extends DataObject> void addDeleteOperationTotTxChain(final LogicalDatastoreType store,
-                                                             final InstanceIdentifier<T> path) throws TransactionChainClosedException {
-        final WriteTransaction writeTx = getTransactionSafely();
-        if (writeTx != null) {
-            LOG.trace("addDeleteOperation called with path {} ", path);
-            writeTx.delete(store, path);
-        } else {
-            LOG.debug("WriteTx is null for node {}. Delete {} was not realized.", nodeII, path);
-            throw new TransactionChainClosedException("Cannot write into transaction.");
+                                                             final InstanceIdentifier<T> path){
+        synchronized (txLock) {
+            ensureTransaction();
+            if (wTx == null) {
+                LOG.debug("WriteTx is null for node {}. Delete {} was not realized.", this.nodeId, path);
+                throw new TransactionChainClosedException(CANNOT_WRITE_INTO_TRANSACTION);
+            }
+
+            wTx.delete(store, path);
         }
     }
 
     <T extends DataObject> void writeToTransaction(final LogicalDatastoreType store,
                                                    final InstanceIdentifier<T> path,
                                                    final T data,
-                                                   final boolean createParents) throws TransactionChainClosedException {
-        final WriteTransaction writeTx = getTransactionSafely();
-        if (writeTx != null) {
-            LOG.trace("writeToTransaction called with path {} ", path);
-            writeTx.put(store, path, data, createParents);
-        } else {
-            LOG.debug("WriteTx is null for node {}. Write data for {} was not realized.", nodeII, path);
-            throw new TransactionChainClosedException("Cannot write into transaction.");
+                                                   final boolean createParents){
+        synchronized (txLock) {
+            ensureTransaction();
+            if (wTx == null) {
+                LOG.debug("WriteTx is null for node {}. Write data for {} was not realized.", this.nodeId, path);
+                throw new TransactionChainClosedException(CANNOT_WRITE_INTO_TRANSACTION);
+            }
+
+            wTx.put(store, path, data, createParents);
         }
     }
 
     @Override
     public void onTransactionChainFailed(final TransactionChain<?, ?> chain,
                                          final AsyncTransaction<?, ?> transaction, final Throwable cause) {
-        if (transactionChainManagerStatus.equals(TransactionChainManagerStatus.WORKING)) {
-            LOG.warn("txChain failed -> recreating due to {}", cause);
-            recreateTxChain();
+        synchronized (txLock) {
+            if (TransactionChainManagerStatus.WORKING == transactionChainManagerStatus) {
+                LOG.warn("Transaction chain failed, recreating chain due to ", cause);
+                createTxChain();
+                wTx = null;
+            }
         }
     }
 
@@ -245,25 +245,13 @@ class TransactionChainManager implements TransactionChainListener, AutoCloseable
         // NOOP
     }
 
-    private void recreateTxChain() {
-        synchronized (txLock) {
-            createTxChain();
-            wTx = null;
-        }
-    }
-
+    @GuardedBy("txLock")
     @Nullable
-    private WriteTransaction getTransactionSafely() {
-        if (wTx == null && TransactionChainManagerStatus.WORKING.equals(transactionChainManagerStatus)) {
-            synchronized (txLock) {
-                if (wTx == null && TransactionChainManagerStatus.WORKING.equals(transactionChainManagerStatus)) {
-                    if (wTx == null && txChainFactory != null) {
-                        wTx = txChainFactory.newWriteOnlyTransaction();
-                    }
-                }
-            }
+    private void ensureTransaction() {
+        if (wTx == null && TransactionChainManagerStatus.WORKING == transactionChainManagerStatus
+            && txChainFactory != null) {
+                wTx = txChainFactory.newWriteOnlyTransaction();
         }
-        return wTx;
     }
 
     @VisibleForTesting
@@ -275,13 +263,13 @@ class TransactionChainManager implements TransactionChainListener, AutoCloseable
     }
 
     ListenableFuture<Void> shuttingDown() {
-        LOG.debug("TxManager is going SHUTTING_DOWN for node {}", nodeII);
-        ListenableFuture<Void> future;
+        if (LOG.isDebugEnabled()) {
+            LOG.debug("TxManager is going SHUTTING_DOWN for node {}", this.nodeId);
+        }
         synchronized (txLock) {
             this.transactionChainManagerStatus = TransactionChainManagerStatus.SHUTTING_DOWN;
-            future = txChainShuttingDown();
+            return txChainShuttingDown();
         }
-        return future;
     }
 
     @GuardedBy("txLock")
@@ -295,6 +283,9 @@ class TransactionChainManager implements TransactionChainListener, AutoCloseable
             // hijack md-sal thread
             future = lastSubmittedFuture;
         } else {
+            if (LOG.isDebugEnabled()) {
+                LOG.debug("Submitting all transactions for Node {}", this.nodeId);
+            }
             // hijack md-sal thread
             future = wTx.submit();
             wTx = null;
@@ -304,17 +295,12 @@ class TransactionChainManager implements TransactionChainListener, AutoCloseable
 
     @Override
     public void close() {
-        LOG.debug("Setting transactionChainManagerStatus to SHUTTING_DOWN for {}, will wait for ownershipservice to notify"
-                , nodeII);
-        Preconditions.checkState(TransactionChainManagerStatus.SHUTTING_DOWN.equals(transactionChainManagerStatus));
-        Preconditions.checkState(wTx == null);
+        if (LOG.isDebugEnabled()) {
+            LOG.debug("Setting transactionChainManagerStatus to SHUTTING_DOWN for {}", this.nodeId);
+        }
         synchronized (txLock) {
-            if (txChainFactory != null) {
-                txChainFactory.close();
-                txChainFactory = null;
-            }
+            removeTxChainFactory();
         }
-        Preconditions.checkState(txChainFactory == null);
     }
 
     private enum TransactionChainManagerStatus {