Make AbstractPingPongTransactionChain.close() idempotent

Expend one byte of state to track close() method, making sure it is
idempotent.

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

diff --git a/dom/mdsal-dom-spi/src/main/java/org/opendaylight/mdsal/dom/spi/AbstractPingPongTransactionChain.java b/dom/mdsal-dom-spi/src/main/java/org/opendaylight/mdsal/dom/spi/AbstractPingPongTransactionChain.java
index 48ebce2c05d889c6efc520151e00f8b3f1dacf3b..fb3a2178a5a6c5abbb3b6021615c978a57ef3512 100644 (file)
--- a/dom/mdsal-dom-spi/src/main/java/org/opendaylight/mdsal/dom/spi/AbstractPingPongTransactionChain.java
+++ b/dom/mdsal-dom-spi/src/main/java/org/opendaylight/mdsal/dom/spi/AbstractPingPongTransactionChain.java
@@ -45,6 +45,8 @@ abstract class AbstractPingPongTransactionChain implements DOMTransactionChain {
     private final DOMTransactionChainListener listener;
     private final DOMTransactionChain delegate;
 
+    @GuardedBy("this")
+    private boolean closed;
     @GuardedBy("this")
     private boolean failed;
     @GuardedBy("this")
@@ -381,12 +383,17 @@ abstract class AbstractPingPongTransactionChain implements DOMTransactionChain {
 
     @Override
     public final synchronized void close() {
-        final PingPongTransaction notLocked = lockedTx;
-        checkState(notLocked == null, "Attempted to close chain with outstanding transaction %s", notLocked);
+        if (closed) {
+            LOG.debug("Attempted to close an already-closed chain");
+            return;
+        }
 
-        // This is not reliable, but if we observe it to be null and the process has already completed,
-        // the backend transaction chain will throw the appropriate error.
-        checkState(shutdownTx == null, "Attempted to close an already-closed chain");
+        // Note: we do not derive from AbstractRegistration due to ordering of this check
+        final var notLocked = lockedTx;
+        if (notLocked != null) {
+            throw new IllegalStateException("Attempted to close chain with outstanding transaction " + notLocked);
+        }
+        closed = true;
 
         // This may be a reaction to our failure callback, in that case the backend is already shutdown
         if (deadTx != null) {
@@ -395,8 +402,7 @@ abstract class AbstractPingPongTransactionChain implements DOMTransactionChain {
         }
 
         // Force allocations on slow path, picking up a potentially-outstanding transaction
-        final PingPongTransaction tx = acquireReadyTx();
-
+        final var tx = acquireReadyTx();
         if (tx != null) {
             // We have one more transaction, which needs to be processed somewhere. If we do not
             // a transaction in-flight, we need to push it down ourselves.

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 60c6d724feca51d3035b6d64d3965eb939655ea1..7b9f49794ef3ce7b37699f6b08249b7d6e3f7e06 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
@@ -24,6 +24,7 @@ import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoMoreInteractions;
 
 import com.google.common.util.concurrent.FluentFuture;
 import com.google.common.util.concurrent.Futures;
@@ -330,6 +331,15 @@ public class PingPongTransactionChainTest {
         assertDone(tx1Future);
     }
 
+    @Test
+    public void testIdempotentClose() {
+        doNothing().when(chain).close();
+        pingPong.close();
+        verify(chain).close();
+        pingPong.close();
+        verifyNoMoreInteractions(chain);
+    }
+
     private static <T> T assertDone(final FluentFuture<T> future) {
         try {
             return Futures.getDone(future);