Fix lead transaction cancellation

We have a thinko around recording frontend transaction, which leads
to us always cancelling the entire chain. This is not correct, as
evidenced by existing tests.

Correct the book keeping and refactor cancelTransaction() to allow
returning correct result of operation and handle the case where backend
refuses to cancel.

JIRA: MDSAL-756
Change-Id: I553e4de2d09acedf67af63bc292fae0bb5dfed78
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>

diff --git a/dom/mdsal-dom-spi/src/main/java/org/opendaylight/mdsal/dom/spi/PingPongTransaction.java b/dom/mdsal-dom-spi/src/main/java/org/opendaylight/mdsal/dom/spi/PingPongTransaction.java
index f57deba0ecae0e16bbb4d957faf7bca67b7328f5..b4472248acb1c6d74e28117d404b571891f34eca 100644 (file)
--- a/dom/mdsal-dom-spi/src/main/java/org/opendaylight/mdsal/dom/spi/PingPongTransaction.java
+++ b/dom/mdsal-dom-spi/src/main/java/org/opendaylight/mdsal/dom/spi/PingPongTransaction.java
@@ -24,16 +24,14 @@ import org.opendaylight.mdsal.dom.api.DOMDataTreeReadWriteTransaction;
  * interface so we have a simple way of propagating the result.
  */
 final class PingPongTransaction implements FutureCallback<CommitInfo> {
+    private final @NonNull SettableFuture<CommitInfo> future = SettableFuture.create();
+    private final @NonNull FluentFuture<CommitInfo> fluent = FluentFuture.from(future);
     private final @NonNull DOMDataTreeReadWriteTransaction delegate;
-    private final @NonNull SettableFuture<CommitInfo> future;
-    private final @NonNull FluentFuture<CommitInfo> fluent;
 
     private @Nullable DOMDataTreeReadWriteTransaction frontendTransaction;
 
     PingPongTransaction(final DOMDataTreeReadWriteTransaction delegate) {
         this.delegate = requireNonNull(delegate);
-        future = SettableFuture.create();
-        fluent = FluentFuture.from(future);
     }
 
     @NonNull DOMDataTreeReadWriteTransaction getTransaction() {
@@ -59,8 +57,8 @@ final class PingPongTransaction implements FutureCallback<CommitInfo> {
     }
 
     void recordFrontendTransaction(final DOMDataTreeReadWriteTransaction tx) {
-        if (frontendTransaction != null) {
-            frontendTransaction = tx;
+        if (frontendTransaction == null) {
+            frontendTransaction = requireNonNull(tx);
         }
     }
 

diff --git a/dom/mdsal-dom-spi/src/main/java/org/opendaylight/mdsal/dom/spi/PingPongTransactionChain.java b/dom/mdsal-dom-spi/src/main/java/org/opendaylight/mdsal/dom/spi/PingPongTransactionChain.java
index 27c47e270b3aae3fc5370c11446fe55d9d3c2af4..2f24b0b947c32fd1641fba60429ad58f9b5ddf81 100644 (file)
--- a/dom/mdsal-dom-spi/src/main/java/org/opendaylight/mdsal/dom/spi/PingPongTransactionChain.java
+++ b/dom/mdsal-dom-spi/src/main/java/org/opendaylight/mdsal/dom/spi/PingPongTransactionChain.java
@@ -342,9 +342,9 @@ public final class PingPongTransactionChain implements DOMTransactionChain {
      *
      * @param tx Backend shared transaction
      * @param frontendTx transaction
-     * @param isOpen indicator whether the transaction was already closed
+     * @return {@code true} if the transaction was cancelled successfully
      */
-    synchronized void cancelTransaction(final PingPongTransaction tx,
+    synchronized boolean cancelTransaction(final PingPongTransaction tx,
             final DOMDataTreeReadWriteTransaction frontendTx) {
         // Attempt to unlock the operation.
         final Object witness = LOCKED_TX.compareAndExchange(this, tx, null);
@@ -356,20 +356,30 @@ public final class PingPongTransactionChain implements DOMTransactionChain {
         if (failed) {
             // The transaction has failed, this is probably the user just clearing up the transaction they had. We have
             // already cancelled the transaction anyway,
-            return;
-        } else if (!backendCancelled) {
-            LOG.warn("Backend transaction cannot be cancelled during cancellation of {}, attempting to continue", tx);
+            return true;
         }
 
-        // We have dealt with canceling the backend transaction and have unlocked the transaction. Since we are still
+        // We have dealt with cancelling the backend transaction and have unlocked the transaction. Since we are still
         // inside the synchronized block, any allocations are blocking on the slow path. Now we have to decide the fate
         // of this transaction chain.
         //
         // If there are no other frontend transactions in this batch we are aligned with backend state and we can
         // continue processing.
         if (frontendTx.equals(tx.getFrontendTransaction())) {
-            LOG.debug("Cancelled transaction {} was head of the batch, resuming processing", tx);
-            return;
+            if (backendCancelled) {
+                LOG.debug("Cancelled transaction {} was head of the batch, resuming processing", tx);
+                return true;
+            }
+
+            // Backend refused to cancel the transaction. Reinstate it to locked state.
+            final Object reinstateWitness = LOCKED_TX.compareAndExchange(this, null, tx);
+            verify(reinstateWitness == null, "Reinstating transaction %s collided with locked transaction %s", tx,
+                reinstateWitness);
+            return false;
+        }
+
+        if (!backendCancelled) {
+            LOG.warn("Backend transaction cannot be cancelled during cancellation of {}, attempting to continue", tx);
         }
 
         // There are multiple frontend transactions in this batch. We have to report them as failed, which dooms this
@@ -378,6 +388,7 @@ public final class PingPongTransactionChain implements DOMTransactionChain {
         // and mark the fact that we should be turning its completion into a failure.
         deadTx = Map.entry(tx, new CancellationException("Transaction " + frontendTx + " canceled").fillInStackTrace());
         delegate.close();
+        return true;
     }
 
     @Override
@@ -481,12 +492,10 @@ public final class PingPongTransactionChain implements DOMTransactionChain {
 
         @Override
         public boolean cancel() {
-            if (isOpen) {
-                cancelTransaction(tx, this);
+            if (isOpen && cancelTransaction(tx, this)) {
                 isOpen = false;
                 return true;
             }
-
             return false;
         }
 

diff --git a/dom/mdsal-dom-spi/src/test/java/org/opendaylight/mdsal/dom/spi/PingPongTransactionChainTest.java b/dom/mdsal-dom-spi/src/test/java/org/opendaylight/mdsal/dom/spi/PingPongTransactionChainTest.java
index ee916f53be2a9aa3d87fa1947813a22854573cb3..a33f4929496bc6a95e73f929a96d9db386ed13d8 100644 (file)
--- a/dom/mdsal-dom-spi/src/test/java/org/opendaylight/mdsal/dom/spi/PingPongTransactionChainTest.java
+++ b/dom/mdsal-dom-spi/src/test/java/org/opendaylight/mdsal/dom/spi/PingPongTransactionChainTest.java
@@ -11,7 +11,6 @@ import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertSame;
 import static org.junit.Assert.assertThrows;
-import static org.junit.Assert.assertTrue;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.doNothing;
@@ -199,15 +198,9 @@ public class PingPongTransactionChainTest {
     private void assertSimpleCancel(final boolean result) {
         final var tx = pingPong.newWriteOnlyTransaction();
 
-        doNothing().when(chain).close();
         doReturn(result).when(rwTx).cancel();
-        doReturn("mock").when(rwTx).toString();
-
-        // FIXME: it seems we are doing the wrong, we should see 'result' returned here
-        assertTrue(tx.cancel());
-
+        assertEquals(result, tx.cancel());
         verify(rwTx).cancel();
-        verify(chain).close();
     }
 
     private static <T> T assertDone(final FluentFuture<T> future) {