BUG-7901: fix unsynchronized transaction access

The evidence in the issue indicates that we have a race condition
where a batch of modifications is being submitted (triggered by
a write/delete and exceeded batch count) and we are observing a
concurrent modification. After auditing the CDS side of this, while
we can improve safety a bit, the scenario has to involve multi-threaded
access by the client.

Looking at TransactionChainManager, it seems to be heavily synchronized,
but there is a hole in the paths which are using getTransactionSafely().

While the access to the TransactionChain, it returns the transaction
(which really is an alias to the wTx field). That throws of static
analysis, as we end up the object pointed to by wTx without holding
the lock -- hence concurrent threads will end up touching the transaction
without any guards, leading to a violation of Transaction contract (which
requires external synchronization).

This patch reworks getTransactionSafely() so it only ensures the field
is initialized as appropriate and makes its callers access the wTx field
with the proper lock held.

Bug: 7500
Bug: 7901
Bug: 8060
Change-Id: I058feb55f91a40ccda6d69ee87cd1a20f792afba
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>
(cherry picked from commit 6dbbd412a88dafdb7d7acd824516850b473b145f)

diff --git a/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/device/TransactionChainManager.java b/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/device/TransactionChainManager.java
index da4bdae04e427d42efd4ab7eebbf44e3b0b27e7d..aded9b26d9ee3d76f6222f9ae172934ddf872885 100644 (file)
--- a/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/device/TransactionChainManager.java
+++ b/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/device/TransactionChainManager.java
@@ -64,7 +64,7 @@ class TransactionChainManager implements TransactionChainListener, AutoCloseable
     @GuardedBy("txLock")
     private ListenableFuture<Void> lastSubmittedFuture;
 
-    private boolean initCommit;
+    private volatile boolean initCommit;
 
     @GuardedBy("txLock")
     private TransactionChainManagerStatus transactionChainManagerStatus = TransactionChainManagerStatus.SLEEPING;
@@ -73,7 +73,6 @@ class TransactionChainManager implements TransactionChainListener, AutoCloseable
                             @Nonnull final DeviceInfo deviceInfo) {
         this.dataBroker = dataBroker;
         this.nodeId = deviceInfo.getNodeInstanceIdentifier().getKey().getId().getValue();
-        this.transactionChainManagerStatus = TransactionChainManagerStatus.SLEEPING;
         this.lastSubmittedFuture = Futures.immediateFuture(null);
     }
 
@@ -103,7 +102,7 @@ class TransactionChainManager implements TransactionChainListener, AutoCloseable
             LOG.debug("activateTransactionManager for node {} transaction submit is set to {}", this.nodeId, submitIsEnabled);
         }
         synchronized (txLock) {
-            if (TransactionChainManagerStatus.SLEEPING.equals(transactionChainManagerStatus)) {
+            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;
@@ -126,7 +125,7 @@ class TransactionChainManager implements TransactionChainListener, AutoCloseable
         }
         final ListenableFuture<Void> future;
         synchronized (txLock) {
-            if (TransactionChainManagerStatus.WORKING.equals(transactionChainManagerStatus)) {
+            if (TransactionChainManagerStatus.WORKING == transactionChainManagerStatus) {
                 transactionChainManagerStatus = TransactionChainManagerStatus.SLEEPING;
                 future = txChainShuttingDown();
                 Preconditions.checkState(wTx == null, "We have some unexpected WriteTransaction.");
@@ -168,15 +167,16 @@ class TransactionChainManager implements TransactionChainListener, AutoCloseable
                 }
                 return true;
             }
-            Preconditions.checkState(TransactionChainManagerStatus.WORKING.equals(transactionChainManagerStatus),
+            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
@@ -192,27 +192,24 @@ class TransactionChainManager implements TransactionChainListener, AutoCloseable
                         }
                     }
                     if (initCommit) {
-                        wTx = null;
                         Optional.ofNullable(lifecycleService).ifPresent(LifecycleService::closeConnection);
                     }
                 }
             });
-            lastSubmittedFuture = submitFuture;
-            wTx = null;
         }
         return true;
     }
 
     <T extends DataObject> void addDeleteOperationTotTxChain(final LogicalDatastoreType store,
                                                              final InstanceIdentifier<T> path){
-        final WriteTransaction writeTx = getTransactionSafely();
-        if (Objects.nonNull(writeTx)) {
-            writeTx.delete(store, path);
-        } else {
-            if (LOG.isDebugEnabled()) {
+        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);
             }
-            throw new TransactionChainClosedException(CANNOT_WRITE_INTO_TRANSACTION);
+
+            wTx.delete(store, path);
         }
     }
 
@@ -220,23 +217,26 @@ class TransactionChainManager implements TransactionChainListener, AutoCloseable
                                                    final InstanceIdentifier<T> path,
                                                    final T data,
                                                    final boolean createParents){
-        final WriteTransaction writeTx = getTransactionSafely();
-        if (Objects.nonNull(writeTx)) {
-            writeTx.put(store, path, data, createParents);
-        } else {
-            if (LOG.isDebugEnabled()) {
+        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);
             }
-            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("Transaction chain failed, recreating chain due to ", cause);
-            recreateTxChain();
+        synchronized (txLock) {
+            if (TransactionChainManagerStatus.WORKING == transactionChainManagerStatus) {
+                LOG.warn("Transaction chain failed, recreating chain due to ", cause);
+                createTxChain();
+                wTx = null;
+            }
         }
     }
 
@@ -245,21 +245,13 @@ class TransactionChainManager implements TransactionChainListener, AutoCloseable
         // NOOP
     }
 
-    private void recreateTxChain() {
-        synchronized (txLock) {
-            createTxChain();
-            wTx = null;
-        }
-    }
-
+    @GuardedBy("txLock")
     @Nullable
-    private WriteTransaction getTransactionSafely() {
-            synchronized (txLock) {
-                if (wTx == null && TransactionChainManagerStatus.WORKING.equals(transactionChainManagerStatus)) {
-                    Optional.ofNullable(txChainFactory).ifPresent(bindingTransactionChain -> wTx = txChainFactory.newWriteOnlyTransaction());
-                }
-            }
-        return wTx;
+    private void ensureTransaction() {
+        if (wTx == null && TransactionChainManagerStatus.WORKING == transactionChainManagerStatus
+            && txChainFactory != null) {
+                wTx = txChainFactory.newWriteOnlyTransaction();
+        }
     }
 
     @VisibleForTesting
@@ -274,12 +266,10 @@ class TransactionChainManager implements TransactionChainListener, AutoCloseable
         if (LOG.isDebugEnabled()) {
             LOG.debug("TxManager is going SHUTTING_DOWN for node {}", this.nodeId);
         }
-        ListenableFuture<Void> future;
         synchronized (txLock) {
             this.transactionChainManagerStatus = TransactionChainManagerStatus.SHUTTING_DOWN;
-            future = txChainShuttingDown();
+            return txChainShuttingDown();
         }
-        return future;
     }
 
     @GuardedBy("txLock")