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 <tpanteli@brocade.com>

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 cb17335caf1b53bcfe2cdfdc2fe13540ad945165..897271a56126084d2ec7b62016f37f1b4900bdaf 100644 (file)
--- 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<DataTreeModification> {
+public final class ReadWriteShardDataTreeTransaction extends AbstractShardDataTreeTransaction<DataTreeModification> {
     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 0d63115754698688f5c64fba9d46d63b93336f87..7c45bd0702db7c07fd6d6a2037c1721204415eab 100644 (file)
--- 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 e1b342a982c89b42955d95ac1007b4486813e810..02283f6aaca6fb73c43569b7fc9aaed63d386512 100644 (file)
--- 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 719f66c018c04c9e6f54c57dd46b4e923e7a9c91..b3d7f2c2d6dea96c5fd507c573ab723983227b1c 100644 (file)
--- 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 a196eda597110baac1dfb5645a6599ec6d7f7e54..7e8d23c6a87c916954090486fe0ea305a7e89625 100644 (file)
--- 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 ad1daff1cfd6bce79aa7de04b3d45028c53c9f9b..725a5f009a260d3198b4f2f520d622ffa62b78f1 100644 (file)
--- 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