From 5efad8eb9dec08e7a0eb00e105385c81598863c2 Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Thu, 28 Apr 2022 18:08:23 +0200 Subject: [PATCH] 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 (cherry picked from commit e781d0af7effb145dc69163744d1a7f02c200d8e) --- .../mdsal/dom/spi/PingPongTransaction.java | 10 +++--- .../dom/spi/PingPongTransactionChain.java | 31 ++++++++++++------- .../dom/spi/PingPongTransactionChainTest.java | 9 +----- 3 files changed, 25 insertions(+), 25 deletions(-) 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 f57deba0ec..b4472248ac 100644 --- 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 { + private final @NonNull SettableFuture future = SettableFuture.create(); + private final @NonNull FluentFuture fluent = FluentFuture.from(future); private final @NonNull DOMDataTreeReadWriteTransaction delegate; - private final @NonNull SettableFuture future; - private final @NonNull FluentFuture 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 { } 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 27c47e270b..2f24b0b947 100644 --- 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 ee916f53be..a33f492949 100644 --- 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 assertDone(final FluentFuture future) { -- 2.36.6