From 67c7314bd62883f9c7e1ae4daa1b6cf70fe3cd73 Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Sun, 3 Sep 2023 21:59:41 +0200 Subject: [PATCH] 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 --- .../spi/AbstractPingPongTransactionChain.java | 20 ++++++++++++------- .../dom/spi/PingPongTransactionChainTest.java | 10 ++++++++++ 2 files changed, 23 insertions(+), 7 deletions(-) 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 48ebce2c05..fb3a2178a5 100644 --- 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 60c6d724fe..7b9f49794e 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 @@ -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 assertDone(final FluentFuture future) { try { return Futures.getDone(future); -- 2.36.6