From: Jaime Caamaño Ruiz Date: Thu, 1 Feb 2018 09:02:46 +0000 (+0100) Subject: Fix infinite loop on cancel transaction X-Git-Tag: release/oxygen~11 X-Git-Url: https://git.opendaylight.org/gerrit/gitweb?p=controller.git;a=commitdiff_plain;h=7ab6f974861e01daa16ff56658eeb1be163cbfec;ds=sidebyside 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 --- 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 5dd8bd3e3e..cfc6c28385 100644 --- 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 submit() { final AbstractDOMTransactionFactory impl = IMPL_UPDATER.getAndSet(this, null); checkRunning(impl); @@ -138,12 +140,17 @@ public abstract class AbstractDOMBrokerWriteTransaction txns = getSubtransactions(); final Collection 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 ret; + try { + for (final T txn : txns) { + cohorts.add(txn.ready()); + } - final CheckedFuture 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 index 0000000000..69265ddc77 --- /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 { + + AbstractDOMBrokerWriteTransactionTestImpl() { + super(new Object(), Collections.emptyMap(), abstractDOMTransactionFactory); + } + + @Override + protected DOMStoreWriteTransaction createTransaction(LogicalDatastoreType key) { + return null; + } + + @Override + protected Collection 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 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 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 1cf28423b5..87743046e1 100644 --- 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 extends } @Override + @SuppressWarnings("checkstyle:illegalcatch") public CheckedFuture submit() { final AbstractDOMForwardedTransactionFactory impl = IMPL_UPDATER.getAndSet(this, null); checkRunning(impl); @@ -143,12 +144,17 @@ class DOMForwardedWriteTransaction extends final Collection txns = getSubtransactions(); final Collection 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 ret; + try { + for (DOMStoreWriteTransaction txn : txns) { + cohorts.add(txn.ready()); + } - final CheckedFuture 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 index 0000000000..9e76b6bfa2 --- /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 domForwardedWriteTransaction = + new DOMForwardedWriteTransaction<>( + new Object(), + Collections.singletonMap(LogicalDatastoreType.OPERATIONAL, domStoreWriteTransaction), + abstractDOMForwardedTransactionFactory); + CheckedFuture 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 domForwardedWriteTransaction = + new DOMForwardedWriteTransaction<>( + new Object(), + Collections.singletonMap(LogicalDatastoreType.OPERATIONAL, domStoreWriteTransaction), + abstractDOMForwardedTransactionFactory); + CheckedFuture 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