BUG-7901: fix unsynchronized transaction access
[openflowplugin.git] / openflowplugin-impl / src / main / java / org / opendaylight / openflowplugin / impl / device / TransactionChainManager.java
index 482d2574ad283bf964f195b18d3973d85b26b997..aded9b26d9ee3d76f6222f9ae172934ddf872885 100644 (file)
@@ -13,6 +13,10 @@ import com.google.common.base.Preconditions;
 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;
@@ -22,15 +26,13 @@ import org.opendaylight.controller.md.sal.binding.api.WriteTransaction;
 import org.opendaylight.controller.md.sal.common.api.data.AsyncTransaction;
 import org.opendaylight.controller.md.sal.common.api.data.LogicalDatastoreType;
 import org.opendaylight.controller.md.sal.common.api.data.TransactionChain;
+import org.opendaylight.controller.md.sal.common.api.data.TransactionChainClosedException;
 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.DeviceState;
-import org.opendaylight.yang.gen.v1.urn.opendaylight.inventory.rev130819.nodes.Node;
-import org.opendaylight.yang.gen.v1.urn.opendaylight.inventory.rev130819.nodes.NodeBuilder;
-import org.opendaylight.yang.gen.v1.urn.opendaylight.inventory.rev130819.nodes.NodeKey;
+import org.opendaylight.openflowplugin.api.openflow.device.DeviceInfo;
+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;
 
@@ -42,48 +44,47 @@ 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 DataBroker dataBroker;
-    private final DeviceState deviceState;
+    private final String nodeId;
+    private LifecycleService lifecycleService;
+
     @GuardedBy("txLock")
     private WriteTransaction wTx;
     @GuardedBy("txLock")
     private BindingTransactionChain txChainFactory;
+    @GuardedBy("txLock")
     private boolean submitIsEnabled;
+    @GuardedBy("txLock")
+    private ListenableFuture<Void> lastSubmittedFuture;
 
-    public TransactionChainManagerStatus getTransactionChainManagerStatus() {
-        return transactionChainManagerStatus;
-    }
+    private volatile boolean initCommit;
 
     @GuardedBy("txLock")
-    private TransactionChainManagerStatus transactionChainManagerStatus;
-    private final KeyedInstanceIdentifier<Node, NodeKey> nodeII;
+    private TransactionChainManagerStatus transactionChainManagerStatus = TransactionChainManagerStatus.SLEEPING;
 
     TransactionChainManager(@Nonnull final DataBroker dataBroker,
-                            @Nonnull final DeviceState deviceState) {
-        this.dataBroker = Preconditions.checkNotNull(dataBroker);
-        this.deviceState = Preconditions.checkNotNull(deviceState);
-        this.nodeII = Preconditions.checkNotNull(deviceState.getNodeInstanceIdentifier());
-        this.transactionChainManagerStatus = TransactionChainManagerStatus.SLEEPING;
-        LOG.debug("created txChainManager");
+                            @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() {
@@ -96,17 +97,18 @@ class TransactionChainManager implements TransactionChainListener, AutoCloseable
      * registration for this class instance as {@link TransactionChainListener} to provide possibility a make DS
      * transactions. Call this method for MASTER role only.
      */
-    public void activateTransactionManager() {
-        LOG.trace("activetTransactionManaager for node {} transaction submit is set to {}", deviceState.getNodeId());
+    void activateTransactionManager() {
+        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 {}", deviceState.getNodeId());
+            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 {}", deviceState.getNodeId());
             }
         }
     }
@@ -117,26 +119,25 @@ class TransactionChainManager implements TransactionChainListener, AutoCloseable
      * Call this method for SLAVE only.
      * @return Future
      */
-    public CheckedFuture<Void, TransactionCommitFailedException> deactivateTransactionManager() {
-        final CheckedFuture<Void, TransactionCommitFailedException> 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", deviceState.getNodeId());
+            if (TransactionChainManagerStatus.WORKING == transactionChainManagerStatus) {
                 transactionChainManagerStatus = TransactionChainManagerStatus.SLEEPING;
                 future = txChainShuttingDown();
                 Preconditions.checkState(wTx == null, "We have some unexpected WriteTransaction.");
-                LOG.debug("Transaction Factory delete for Node {}", deviceState.getNodeId());
                 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 {
@@ -147,65 +148,95 @@ class TransactionChainManager implements TransactionChainListener, AutoCloseable
         return future;
     }
 
+    private void removeTxChainFactory() {
+        Optional.ofNullable(txChainFactory).ifPresent(TransactionChain::close);
+        txChainFactory = null;
+    }
+
     boolean submitWriteTransaction() {
-        if (!submitIsEnabled) {
-            LOG.trace("transaction not committed - submit block issued");
-            return false;
-        }
         synchronized (txLock) {
-            if (wTx == null) {
-                LOG.trace("nothing to commit - submit returns true");
+            if (!submitIsEnabled) {
+                if (LOG.isTraceEnabled()) {
+                    LOG.trace("transaction not committed - submit block issued");
+                }
+                return false;
+            }
+            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) {
-                    //no action required
+                    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) {
+                        Optional.ofNullable(lifecycleService).ifPresent(LifecycleService::closeConnection);
                     }
                 }
             });
-            wTx = null;
         }
         return true;
     }
 
     <T extends DataObject> void addDeleteOperationTotTxChain(final LogicalDatastoreType store,
-                                                             final InstanceIdentifier<T> path) {
-        final WriteTransaction writeTx = getTransactionSafely();
-        if (writeTx != null) {
-            writeTx.delete(store, path);
-        } else {
-            LOG.debug("WriteTx is null for node {}. Delete {} was not realized.", nodeII, path);
+                                                             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 WriteTransaction writeTx = getTransactionSafely();
-        if (writeTx != null) {
-            writeTx.put(store, path, data);
-        } else {
-            LOG.debug("WriteTx is null for node {}. Write data for {} was not realized.", nodeII, path);
+                                                   final InstanceIdentifier<T> path,
+                                                   final T data,
+                                                   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", cause);
-            recreateTxChain();
+        synchronized (txLock) {
+            if (TransactionChainManagerStatus.WORKING == transactionChainManagerStatus) {
+                LOG.warn("Transaction chain failed, recreating chain due to ", cause);
+                createTxChain();
+                wTx = null;
+            }
         }
     }
 
@@ -214,54 +245,48 @@ 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
     void enableSubmit() {
-        submitIsEnabled = true;
+        synchronized (txLock) {
+            /* !!!IMPORTANT: never set true without txChainFactory */
+            submitIsEnabled = txChainFactory != null;
+        }
     }
 
-    CheckedFuture<Void, TransactionCommitFailedException> shuttingDown() {
-        LOG.debug("TxManager is going SUTTING_DOWN for node {}", nodeII);
-        CheckedFuture<Void, TransactionCommitFailedException> future;
+    ListenableFuture<Void> shuttingDown() {
+        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;
     }
 
-    private CheckedFuture<Void, TransactionCommitFailedException> txChainShuttingDown() {
-        CheckedFuture<Void, TransactionCommitFailedException> future;
+    @GuardedBy("txLock")
+    private ListenableFuture<Void> txChainShuttingDown() {
+        submitIsEnabled = false;
+        ListenableFuture<Void> future;
         if (txChainFactory == null) {
             // stay with actual thread
             future = Futures.immediateCheckedFuture(null);
-        } else {
+        } else if (wTx == null) {
             // hijack md-sal thread
-            if (wTx == null) {
-                wTx = txChainFactory.newWriteOnlyTransaction();
+            future = lastSubmittedFuture;
+        } else {
+            if (LOG.isDebugEnabled()) {
+                LOG.debug("Submitting all transactions for Node {}", this.nodeId);
             }
-            final NodeBuilder nodeBuilder = new NodeBuilder().setId(deviceState.getNodeId());
-            wTx.merge(LogicalDatastoreType.OPERATIONAL, nodeII, nodeBuilder.build());
+            // hijack md-sal thread
             future = wTx.submit();
             wTx = null;
         }
@@ -270,19 +295,15 @@ class TransactionChainManager implements TransactionChainListener, AutoCloseable
 
     @Override
     public void close() {
-        LOG.debug("Setting transactionChainManagerStatus to SHUTTING_DOWN, 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);
     }
 
-    public enum TransactionChainManagerStatus {
+    private enum TransactionChainManagerStatus {
         /** txChainManager is sleeping - is not active (SLAVE or default init value) */
         WORKING,
         /** txChainManager is working - is active (MASTER) */