From 32681853a2d71d7a6bc891091e9820918b1541d2 Mon Sep 17 00:00:00 2001 From: Tom Pantelis Date: Thu, 10 Dec 2015 18:31:21 -0500 Subject: [PATCH] Move Tx ready call from ShardWriteTransaction to Shard On ready, the ShardWriteTransaction actor calls ready on the ReadWriteShardDataTreeTransaction to obtain the ShardDataTreeCohort and passes it with the ForwardedReadyTransaction message sent to the Shard. Internally the ReadWriteShardDataTreeTransaction accesses its parent, which is either the ShardDataTree or a ShardDataTreeTransactionChain, to obtain the ShardDataTreeCohort. The parent holds unsynchronized internal Shard state and is potentially unsafe to indirectly leak to the transaction actor. This isn't an issue with the ShardDataTree b/c finishTransaction doesn't access internal state but ShardDataTreeTransactionChain does. To be safe, we need to move the ready call to the Shard. Therefore, the ReadWriteShardDataTreeTransaction instance is now passed with the ForwardedReadyTransaction and the ShardCommitCoordinator now calls ready. Change-Id: I59295936944326b68506380519f8ade3b11164d9 Signed-off-by: Tom Pantelis --- .../ReadWriteShardDataTreeTransaction.java | 2 +- .../datastore/ShardCommitCoordinator.java | 3 ++- .../cluster/datastore/ShardWriteTransaction.java | 4 +--- .../messages/ForwardedReadyTransaction.java | 13 +++++++------ .../cluster/datastore/AbstractShardTest.java | 16 +++++++++++++++- .../datastore/compat/PreLithiumShardTest.java | 10 +++------- 6 files changed, 29 insertions(+), 19 deletions(-) diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ReadWriteShardDataTreeTransaction.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ReadWriteShardDataTreeTransaction.java index cb17335caf..897271a561 100644 --- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ReadWriteShardDataTreeTransaction.java +++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ReadWriteShardDataTreeTransaction.java @@ -10,7 +10,7 @@ package org.opendaylight.controller.cluster.datastore; import com.google.common.base.Preconditions; import org.opendaylight.yangtools.yang.data.api.schema.tree.DataTreeModification; -final class ReadWriteShardDataTreeTransaction extends AbstractShardDataTreeTransaction { +public final class ReadWriteShardDataTreeTransaction extends AbstractShardDataTreeTransaction { private final ShardDataTreeTransactionParent parent; protected ReadWriteShardDataTreeTransaction(final ShardDataTreeTransactionParent parent, final String id, final DataTreeModification modification) { diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardCommitCoordinator.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardCommitCoordinator.java index 0d63115754..7c45bd0702 100644 --- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardCommitCoordinator.java +++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardCommitCoordinator.java @@ -127,7 +127,8 @@ class ShardCommitCoordinator { log.debug("{}: Readying transaction {}, client version {}", name, ready.getTransactionID(), ready.getTxnClientVersion()); - CohortEntry cohortEntry = new CohortEntry(ready.getTransactionID(), ready.getCohort()); + ShardDataTreeCohort cohort = ready.getTransaction().ready(); + CohortEntry cohortEntry = new CohortEntry(ready.getTransactionID(), cohort); cohortCache.put(ready.getTransactionID(), cohortEntry); if(!queueCohortEntry(cohortEntry, sender, shard)) { diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardWriteTransaction.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardWriteTransaction.java index e1b342a982..02283f6aac 100644 --- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardWriteTransaction.java +++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardWriteTransaction.java @@ -178,10 +178,8 @@ public class ShardWriteTransaction extends ShardTransaction { LOG.debug("readyTransaction : {}", transactionID); - ShardDataTreeCohort cohort = transaction.ready(); - getShardActor().forward(new ForwardedReadyTransaction(transactionID, getClientTxVersion(), - cohort, returnSerialized, doImmediateCommit), getContext()); + transaction, returnSerialized, doImmediateCommit), getContext()); // The shard will handle the commit from here so we're no longer needed - self-destruct. getSelf().tell(PoisonPill.getInstance(), getSelf()); diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/messages/ForwardedReadyTransaction.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/messages/ForwardedReadyTransaction.java index 719f66c018..b3d7f2c2d6 100644 --- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/messages/ForwardedReadyTransaction.java +++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/messages/ForwardedReadyTransaction.java @@ -7,7 +7,8 @@ */ package org.opendaylight.controller.cluster.datastore.messages; -import org.opendaylight.controller.cluster.datastore.ShardDataTreeCohort; +import com.google.common.base.Preconditions; +import org.opendaylight.controller.cluster.datastore.ReadWriteShardDataTreeTransaction; /** * Transaction ReadyTransaction message that is forwarded to the local Shard from the ShardTransaction. @@ -16,16 +17,16 @@ import org.opendaylight.controller.cluster.datastore.ShardDataTreeCohort; */ public class ForwardedReadyTransaction { private final String transactionID; - private final ShardDataTreeCohort cohort; + private final ReadWriteShardDataTreeTransaction transaction; private final boolean returnSerialized; private final boolean doImmediateCommit; private final short txnClientVersion; public ForwardedReadyTransaction(String transactionID, short txnClientVersion, - ShardDataTreeCohort cohort, boolean returnSerialized, + ReadWriteShardDataTreeTransaction transaction, boolean returnSerialized, boolean doImmediateCommit) { this.transactionID = transactionID; - this.cohort = cohort; + this.transaction = Preconditions.checkNotNull(transaction); this.returnSerialized = returnSerialized; this.txnClientVersion = txnClientVersion; this.doImmediateCommit = doImmediateCommit; @@ -35,8 +36,8 @@ public class ForwardedReadyTransaction { return transactionID; } - public ShardDataTreeCohort getCohort() { - return cohort; + public ReadWriteShardDataTreeTransaction getTransaction() { + return transaction; } public boolean isReturnSerialized() { diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/AbstractShardTest.java b/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/AbstractShardTest.java index a196eda597..7e8d23c6a8 100644 --- a/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/AbstractShardTest.java +++ b/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/AbstractShardTest.java @@ -11,7 +11,10 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import static org.mockito.Matchers.any; import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.doNothing; +import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; import static org.opendaylight.controller.cluster.datastore.DataStoreVersions.CURRENT_VERSION; import akka.actor.ActorRef; @@ -253,13 +256,24 @@ public abstract class AbstractShardTest extends AbstractActorTest{ MutableCompositeModification modification, boolean doCommitOnReady) { if(remoteReadWriteTransaction){ - return new ForwardedReadyTransaction(transactionID, CURRENT_VERSION, cohort, true, doCommitOnReady); + return prepareForwardedReadyTransaction(cohort, transactionID, CURRENT_VERSION, + doCommitOnReady); } else { setupCohortDecorator(shard, cohort); return prepareBatchedModifications(transactionID, modification, doCommitOnReady); } } + protected Object prepareForwardedReadyTransaction(ShardDataTreeCohort cohort, String transactionID, + short version, boolean doCommitOnReady) { + ShardDataTreeTransactionParent mockParent = mock(ShardDataTreeTransactionParent.class); + doReturn(cohort).when(mockParent).finishTransaction(any(ReadWriteShardDataTreeTransaction.class)); + doNothing().when(mockParent).abortTransaction(any(AbstractShardDataTreeTransaction.class)); + return new ForwardedReadyTransaction(transactionID, version, + new ReadWriteShardDataTreeTransaction(mockParent, transactionID, + mock(DataTreeModification.class)), true, doCommitOnReady); + } + protected Object prepareReadyTransactionMessage(boolean remoteReadWriteTransaction, Shard shard, ShardDataTreeCohort cohort, String transactionID, MutableCompositeModification modification) { diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/compat/PreLithiumShardTest.java b/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/compat/PreLithiumShardTest.java index ad1daff1cf..725a5f009a 100644 --- a/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/compat/PreLithiumShardTest.java +++ b/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/compat/PreLithiumShardTest.java @@ -38,7 +38,6 @@ import org.opendaylight.controller.cluster.datastore.messages.CanCommitTransacti import org.opendaylight.controller.cluster.datastore.messages.CanCommitTransactionReply; import org.opendaylight.controller.cluster.datastore.messages.CommitTransaction; import org.opendaylight.controller.cluster.datastore.messages.CommitTransactionReply; -import org.opendaylight.controller.cluster.datastore.messages.ForwardedReadyTransaction; import org.opendaylight.controller.cluster.datastore.messages.ReadyTransactionReply; import org.opendaylight.controller.cluster.datastore.modification.MergeModification; import org.opendaylight.controller.cluster.datastore.modification.Modification; @@ -250,8 +249,7 @@ public class PreLithiumShardTest extends AbstractShardTest { // Simulate the ForwardedReadyTransaction message for the first Tx that would be sent // by the ShardTransaction. - shard.tell(new ForwardedReadyTransaction(transactionID1, HELIUM_2_VERSION, - cohort1, true, false), getRef()); + shard.tell(prepareForwardedReadyTransaction(cohort1, transactionID1, HELIUM_2_VERSION, false), getRef()); ReadyTransactionReply readyReply = ReadyTransactionReply.fromSerializable( expectMsgClass(duration, ReadyTransactionReply.SERIALIZABLE_CLASS)); assertEquals("Cohort path", shard.path().toString(), readyReply.getCohortPath()); @@ -265,12 +263,10 @@ public class PreLithiumShardTest extends AbstractShardTest { // Send the ForwardedReadyTransaction for the next 2 Tx's. - shard.tell(new ForwardedReadyTransaction(transactionID2, HELIUM_2_VERSION, - cohort2, true, false), getRef()); + shard.tell(prepareForwardedReadyTransaction(cohort2, transactionID2, HELIUM_2_VERSION, false), getRef()); expectMsgClass(duration, ReadyTransactionReply.SERIALIZABLE_CLASS); - shard.tell(new ForwardedReadyTransaction(transactionID3, HELIUM_2_VERSION, - cohort3, true, false), getRef()); + shard.tell(prepareForwardedReadyTransaction(cohort3, transactionID3, HELIUM_2_VERSION, false), getRef()); expectMsgClass(duration, ReadyTransactionReply.SERIALIZABLE_CLASS); // Send the CanCommitTransaction message for the next 2 Tx's. These should get queued and -- 2.36.6