Fix infinite loop on cancel transaction

This patch fixes a problem where you would run into an infinite loop
after cancelling DOMForwardedWriteTransaction following an exception
thrown by the backed transaction ready or submit methods.

Change-Id: I24ce3706dcc52e35890246b4796090cd6b1c99b9
JIRA: CONTROLLER-1812
Signed-off-by: Jaime Caamaño Ruiz <jcaamano@suse.com>

diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/databroker/AbstractDOMBrokerWriteTransaction.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/databroker/AbstractDOMBrokerWriteTransaction.java
index 5dd8bd3e3ed7fcb0b42b3fd3acb6a32f2f229d65..cfc6c283856f8810e67430de02aaab9a45fd9aa3 100644 (file)
--- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/databroker/AbstractDOMBrokerWriteTransaction.java
+++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/databroker/AbstractDOMBrokerWriteTransaction.java
@@ -22,6 +22,7 @@ import org.opendaylight.controller.md.sal.common.api.data.LogicalDatastoreType;
 import org.opendaylight.controller.md.sal.common.api.data.TransactionCommitFailedException;
 import org.opendaylight.controller.md.sal.common.impl.service.AbstractDataTransaction;
 import org.opendaylight.controller.md.sal.dom.api.DOMDataWriteTransaction;
+import org.opendaylight.controller.md.sal.dom.broker.impl.TransactionCommitFailedExceptionMapper;
 import org.opendaylight.controller.sal.core.spi.data.DOMStoreThreePhaseCommitCohort;
 import org.opendaylight.controller.sal.core.spi.data.DOMStoreTransactionFactory;
 import org.opendaylight.controller.sal.core.spi.data.DOMStoreWriteTransaction;
@@ -131,6 +132,7 @@ public abstract class AbstractDOMBrokerWriteTransaction<T extends DOMStoreWriteT
     }
 
     @Override
+    @SuppressWarnings("checkstyle:illegalcatch")
     public CheckedFuture<Void, TransactionCommitFailedException> submit() {
         final AbstractDOMTransactionFactory<?> impl = IMPL_UPDATER.getAndSet(this, null);
         checkRunning(impl);
@@ -138,12 +140,17 @@ public abstract class AbstractDOMBrokerWriteTransaction<T extends DOMStoreWriteT
         final Collection<T> txns = getSubtransactions();
         final Collection<DOMStoreThreePhaseCommitCohort> cohorts = new ArrayList<>(txns.size());
 
-        // FIXME: deal with errors thrown by backed (ready and submit can fail in theory)
-        for (final T txn : txns) {
-            cohorts.add(txn.ready());
-        }
+        CheckedFuture<Void, TransactionCommitFailedException> ret;
+        try {
+            for (final T txn : txns) {
+                cohorts.add(txn.ready());
+            }
 
-        final CheckedFuture<Void, TransactionCommitFailedException> ret = impl.submit(this, cohorts);
+            ret = impl.submit(this, cohorts);
+        } catch (RuntimeException e) {
+            ret = Futures.immediateFailedCheckedFuture(
+                    TransactionCommitFailedExceptionMapper.COMMIT_ERROR_MAPPER.apply(e));
+        }
         FUTURE_UPDATER.lazySet(this, ret);
         return ret;
     }

diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/databroker/AbstractDOMBrokerWriteTransactionTest.java b/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/databroker/AbstractDOMBrokerWriteTransactionTest.java
new file mode 100644 (file)
index 0000000..69265dd
--- /dev/null
+++ b/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/databroker/AbstractDOMBrokerWriteTransactionTest.java
@@ -0,0 +1,91 @@
+/*
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License v1.0 which accompanies this distribution,
+ * and is available at http://www.eclipse.org/legal/epl-v10.html
+ */
+package org.opendaylight.controller.cluster.databroker;
+
+import static org.junit.Assert.assertTrue;
+import static org.mockito.Matchers.any;
+import static org.mockito.Mockito.doThrow;
+
+import com.google.common.util.concurrent.CheckedFuture;
+import java.util.Collection;
+import java.util.Collections;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
+import org.opendaylight.controller.md.sal.common.api.data.LogicalDatastoreType;
+import org.opendaylight.controller.md.sal.common.api.data.TransactionCommitFailedException;
+import org.opendaylight.controller.sal.core.spi.data.DOMStoreWriteTransaction;
+
+public class AbstractDOMBrokerWriteTransactionTest {
+
+    @Mock
+    private AbstractDOMTransactionFactory abstractDOMTransactionFactory;
+
+    @Mock
+    private DOMStoreWriteTransaction domStoreWriteTransaction;
+
+    private class AbstractDOMBrokerWriteTransactionTestImpl
+            extends AbstractDOMBrokerWriteTransaction<DOMStoreWriteTransaction> {
+
+        AbstractDOMBrokerWriteTransactionTestImpl() {
+            super(new Object(), Collections.emptyMap(), abstractDOMTransactionFactory);
+        }
+
+        @Override
+        protected DOMStoreWriteTransaction createTransaction(LogicalDatastoreType key) {
+            return null;
+        }
+
+        @Override
+        protected Collection<DOMStoreWriteTransaction> getSubtransactions() {
+            return Collections.singletonList(domStoreWriteTransaction);
+        }
+    }
+
+    @Before
+    public void setup() {
+        MockitoAnnotations.initMocks(this);
+    }
+
+    @Test
+    public void readyRuntimeExceptionAndCancel() {
+        RuntimeException thrown = new RuntimeException();
+        doThrow(thrown).when(domStoreWriteTransaction).ready();
+        AbstractDOMBrokerWriteTransactionTestImpl abstractDOMBrokerWriteTransactionTestImpl =
+                new AbstractDOMBrokerWriteTransactionTestImpl();
+
+        CheckedFuture<Void, TransactionCommitFailedException> submitFuture =
+                abstractDOMBrokerWriteTransactionTestImpl.submit();
+        try {
+            submitFuture.checkedGet();
+            Assert.fail("TransactionCommitFailedException expected");
+        } catch (TransactionCommitFailedException e) {
+            assertTrue(e.getCause() == thrown);
+            abstractDOMBrokerWriteTransactionTestImpl.cancel();
+        }
+    }
+
+    @Test
+    public void submitRuntimeExceptionAndCancel() {
+        RuntimeException thrown = new RuntimeException();
+        doThrow(thrown).when(abstractDOMTransactionFactory).submit(any(), any());
+        AbstractDOMBrokerWriteTransactionTestImpl abstractDOMBrokerWriteTransactionTestImpl
+                = new AbstractDOMBrokerWriteTransactionTestImpl();
+
+        CheckedFuture<Void, TransactionCommitFailedException> submitFuture =
+                abstractDOMBrokerWriteTransactionTestImpl.submit();
+        try {
+            submitFuture.checkedGet();
+            Assert.fail("TransactionCommitFailedException expected");
+        } catch (TransactionCommitFailedException e) {
+            assertTrue(e.getCause() == thrown);
+            abstractDOMBrokerWriteTransactionTestImpl.cancel();
+        }
+    }
+}
\ No newline at end of file

diff --git a/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/broker/impl/DOMForwardedWriteTransaction.java b/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/broker/impl/DOMForwardedWriteTransaction.java
index 1cf28423b51ef4343325c3f33255a76ec4f9034e..87743046e122fbdf423db3b0faacae11e9ee5fa2 100644 (file)
--- a/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/broker/impl/DOMForwardedWriteTransaction.java
+++ b/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/broker/impl/DOMForwardedWriteTransaction.java
@@ -136,6 +136,7 @@ class DOMForwardedWriteTransaction<T extends DOMStoreWriteTransaction> extends
     }
 
     @Override
+    @SuppressWarnings("checkstyle:illegalcatch")
     public CheckedFuture<Void, TransactionCommitFailedException> submit() {
         final AbstractDOMForwardedTransactionFactory<?> impl = IMPL_UPDATER.getAndSet(this, null);
         checkRunning(impl);
@@ -143,12 +144,17 @@ class DOMForwardedWriteTransaction<T extends DOMStoreWriteTransaction> extends
         final Collection<T> txns = getSubtransactions();
         final Collection<DOMStoreThreePhaseCommitCohort> cohorts = new ArrayList<>(txns.size());
 
-        // FIXME: deal with errors thrown by backed (ready and submit can fail in theory)
-        for (DOMStoreWriteTransaction txn : txns) {
-            cohorts.add(txn.ready());
-        }
+        CheckedFuture<Void, TransactionCommitFailedException> ret;
+        try {
+            for (DOMStoreWriteTransaction txn : txns) {
+                cohorts.add(txn.ready());
+            }
 
-        final CheckedFuture<Void, TransactionCommitFailedException> ret = impl.submit(this, cohorts);
+            ret = impl.submit(this, cohorts);
+        } catch (RuntimeException e) {
+            ret = Futures.immediateFailedCheckedFuture(
+                    TransactionCommitFailedExceptionMapper.COMMIT_ERROR_MAPPER.apply(e));
+        }
         FUTURE_UPDATER.lazySet(this, ret);
         return ret;
     }

diff --git a/opendaylight/md-sal/sal-dom-broker/src/test/java/org/opendaylight/controller/md/sal/dom/broker/impl/DOMForwardedWriteTransactionTest.java b/opendaylight/md-sal/sal-dom-broker/src/test/java/org/opendaylight/controller/md/sal/dom/broker/impl/DOMForwardedWriteTransactionTest.java
new file mode 100644 (file)
index 0000000..9e76b6b
--- /dev/null
+++ b/opendaylight/md-sal/sal-dom-broker/src/test/java/org/opendaylight/controller/md/sal/dom/broker/impl/DOMForwardedWriteTransactionTest.java
@@ -0,0 +1,76 @@
+/*
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License v1.0 which accompanies this distribution,
+ * and is available at http://www.eclipse.org/legal/epl-v10.html
+ */
+package org.opendaylight.controller.md.sal.dom.broker.impl;
+
+import static org.junit.Assert.assertTrue;
+import static org.mockito.Matchers.any;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.doThrow;
+
+import com.google.common.util.concurrent.CheckedFuture;
+import java.util.Collections;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
+import org.opendaylight.controller.md.sal.common.api.data.LogicalDatastoreType;
+import org.opendaylight.controller.md.sal.common.api.data.TransactionCommitFailedException;
+import org.opendaylight.controller.sal.core.spi.data.DOMStoreWriteTransaction;
+
+public class DOMForwardedWriteTransactionTest {
+
+    @Mock
+    private AbstractDOMForwardedTransactionFactory abstractDOMForwardedTransactionFactory;
+
+    @Mock
+    private DOMStoreWriteTransaction domStoreWriteTransaction;
+
+    @Before
+    public void setup() {
+        MockitoAnnotations.initMocks(this);
+    }
+
+    @Test
+    public void readyRuntimeExceptionAndCancel() {
+        RuntimeException thrown = new RuntimeException();
+        doThrow(thrown).when(domStoreWriteTransaction).ready();
+        DOMForwardedWriteTransaction<DOMStoreWriteTransaction> domForwardedWriteTransaction =
+                new DOMForwardedWriteTransaction<>(
+                        new Object(),
+                        Collections.singletonMap(LogicalDatastoreType.OPERATIONAL, domStoreWriteTransaction),
+                        abstractDOMForwardedTransactionFactory);
+        CheckedFuture<Void, TransactionCommitFailedException> submitFuture = domForwardedWriteTransaction.submit();
+        try {
+            submitFuture.checkedGet();
+            Assert.fail("TransactionCommitFailedException expected");
+        } catch (TransactionCommitFailedException e) {
+            assertTrue(e.getCause() == thrown);
+            domForwardedWriteTransaction.cancel();
+        }
+    }
+
+    @Test
+    public void submitRuntimeExceptionAndCancel() {
+        RuntimeException thrown = new RuntimeException();
+        doReturn(null).when(domStoreWriteTransaction).ready();
+        doThrow(thrown).when(abstractDOMForwardedTransactionFactory).submit(any(), any());
+        DOMForwardedWriteTransaction<DOMStoreWriteTransaction> domForwardedWriteTransaction =
+                new DOMForwardedWriteTransaction<>(
+                    new Object(),
+                    Collections.singletonMap(LogicalDatastoreType.OPERATIONAL, domStoreWriteTransaction),
+                    abstractDOMForwardedTransactionFactory);
+        CheckedFuture<Void, TransactionCommitFailedException> submitFuture = domForwardedWriteTransaction.submit();
+        try {
+            submitFuture.checkedGet();
+            Assert.fail("TransactionCommitFailedException expected");
+        } catch (TransactionCommitFailedException e) {
+            assertTrue(e.getCause() == thrown);
+            domForwardedWriteTransaction.cancel();
+        }
+    }
+}
\ No newline at end of file