Bug 7901: Prevent null Modification in BatchedModifications

Caused by: java.lang.NullPointerException
   at org.opendaylight.controller.cluster.datastore.modification.
      MutableCompositeModification.writeExternal(MutableCompositeModification.java:120)

Somehow a null Modification instance got added to the modifications List.
Looking at the callers of addModification, it is not clear which call site
might have been the culprit. So I added a guard in addModification against
a null input Modification and logged an error with stack trace. I also
modified getModifications to return an immutable List to prevent callers
from directly modifying the List (there was one).

Change-Id: Ic63aa9daada0548da05fe663a0d22cdcb3e7bceb
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/RemoteTransactionContext.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/RemoteTransactionContext.java
index 6334bdcd0731e8e2d833cef4af0a7463c524062b..ba64e29d7542ec2027c4d6f1cbabd11d54bd5af0 100644 (file)
--- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/RemoteTransactionContext.java
+++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/RemoteTransactionContext.java
@@ -139,13 +139,15 @@ public class RemoteTransactionContext extends AbstractTransactionContext {
             batchedModifications.setReady(ready);
             batchedModifications.setDoCommitOnReady(doCommitOnReady);
             batchedModifications.setTotalMessagesSent(++totalBatchedModificationsSent);
-            sent = executeOperationAsync(batchedModifications, actorContext.getTransactionCommitOperationTimeout());
 
+            final BatchedModifications toSend = batchedModifications;
             if (ready) {
                 batchedModifications = null;
             } else {
                 batchedModifications = newBatchedModifications();
             }
+
+            sent = executeOperationAsync(toSend, actorContext.getTransactionCommitOperationTimeout());
         }
 
         return sent;

diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnershipShardCommitCoordinator.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnershipShardCommitCoordinator.java
index 9c65ed71d268e2d4a786ac680ccb744570b911dd..a6958297443e9cb7e635f300a96c1b3346e715e6 100644 (file)
--- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnershipShardCommitCoordinator.java
+++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnershipShardCommitCoordinator.java
@@ -266,7 +266,7 @@ class EntityOwnershipShardCommitCoordinator {
 
     private void newInflightCommitWithDifferentTransactionID() {
         BatchedModifications newBatchedModifications = newBatchedModifications();
-        newBatchedModifications.getModifications().addAll(inflightCommit.getModifications());
+        newBatchedModifications.addModifications(inflightCommit.getModifications());
         inflightCommit = newBatchedModifications;
     }
 

diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/modification/CompositeModification.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/modification/CompositeModification.java
index 2bc6d1d84c9bd49a43d261bb4576eec31ebcf663..ced960db5cd203195960a192a07af2aa6d553145 100644 (file)
--- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/modification/CompositeModification.java
+++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/modification/CompositeModification.java
@@ -21,7 +21,7 @@ public interface CompositeModification extends Modification {
     /**
      * Get a list of modifications contained by this composite.
      *
-     * @return list of modifications
+     * @return an immutable list of modifications
      */
     List<Modification> getModifications();
 }

diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/modification/MutableCompositeModification.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/modification/MutableCompositeModification.java
index 6372b21befbb02cd81f30658c8f67f64fcc59a1e..f2a8c00d19035120b8bcac7b522d3d97465cd29e 100644 (file)
--- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/modification/MutableCompositeModification.java
+++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/modification/MutableCompositeModification.java
@@ -13,6 +13,7 @@ import java.io.IOException;
 import java.io.ObjectInput;
 import java.io.ObjectOutput;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.List;
 import org.opendaylight.controller.cluster.datastore.DataStoreVersions;
 import org.opendaylight.controller.cluster.datastore.messages.VersionedExternalizableMessage;
@@ -29,6 +30,7 @@ public class MutableCompositeModification extends VersionedExternalizableMessage
     private static final long serialVersionUID = 1L;
 
     private final List<Modification> modifications = new ArrayList<>();
+    private List<Modification> immutableModifications = null;
 
     public MutableCompositeModification() {
         this(DataStoreVersions.CURRENT_VERSION);
@@ -63,12 +65,23 @@ public class MutableCompositeModification extends VersionedExternalizableMessage
      * @param modification the modification to add.
      */
     public void addModification(Modification modification) {
+        Preconditions.checkNotNull(modification);
         modifications.add(modification);
     }
 
+    public void addModifications(Iterable<Modification> newMods) {
+        for (Modification mod : newMods) {
+            addModification(mod);
+        }
+    }
+
     @Override
     public List<Modification> getModifications() {
-        return modifications;
+        if (immutableModifications == null) {
+            immutableModifications = Collections.unmodifiableList(modifications);
+        }
+
+        return immutableModifications;
     }
 
     @Override