Usage of Collections.unmodifiableCollection is unsafe

This is a follow-up to https://git.opendaylight.org/gerrit/#/c/51583/
wrt a review comment questioning why I returned a new ArrayList instead
of returning the Set directly. One reason was to avoid mutation of the
internal set by the caller but also to capture the state of the set at
that point in time and avoid concurrent mods which may not be safe.
Where a concurrent set is used, it would be thread-safe to return the
set directly but the set may by modified as the caller is iterating it
which may not be desired. For the other class whose set is a keySet
from a non-threadsafe Map, the caller could get a ConcurrentMod
exception while iterating. Based on the review, I changed the call
sites to return a Collections.unmodifiableCollection but this is also
incorrect and is susceptible to the same issues as that impl reads-thru
to the underlying collection. Therefore I changed the call sites back
to returning a new ArrayList.

Change-Id: I504f38c5bfc4c707180ac301eb10acd0ac24f872
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/DataChangeListenerSupport.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/DataChangeListenerSupport.java
index 9b2beccdad99740cbcba027e4a893d8cf9555133..2e26e6ee36d170d4bb0d3da3ae3d276d8f62305c 100644 (file)
--- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/DataChangeListenerSupport.java
+++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/DataChangeListenerSupport.java
@@ -11,8 +11,8 @@ import akka.actor.ActorRef;
 import akka.actor.ActorSelection;
 import com.google.common.base.Optional;
 import com.google.common.collect.Sets;
+import java.util.ArrayList;
 import java.util.Collection;
-import java.util.Collections;
 import java.util.Map.Entry;
 import java.util.Set;
 import org.opendaylight.controller.cluster.datastore.messages.EnableNotification;
@@ -38,7 +38,7 @@ final class DataChangeListenerSupport extends AbstractDataListenerSupport<
     }
 
     Collection<ActorSelection> getListenerActors() {
-        return Collections.unmodifiableCollection(listenerActors);
+        return new ArrayList<>(listenerActors);
     }
 
     @Override

diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/DataTreeChangeListenerSupport.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/DataTreeChangeListenerSupport.java
index 5b8e5f8abfb871f4bd2c503ce7de1853b4c5aac5..9a44b47e7e9257222804ed46c1e08ccaabdbf322 100644 (file)
--- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/DataTreeChangeListenerSupport.java
+++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/DataTreeChangeListenerSupport.java
@@ -11,8 +11,8 @@ import akka.actor.ActorRef;
 import akka.actor.ActorSelection;
 import com.google.common.base.Optional;
 import com.google.common.collect.Sets;
+import java.util.ArrayList;
 import java.util.Collection;
-import java.util.Collections;
 import java.util.Map.Entry;
 import java.util.Set;
 import org.opendaylight.controller.cluster.datastore.messages.EnableNotification;
@@ -33,7 +33,7 @@ final class DataTreeChangeListenerSupport extends AbstractDataListenerSupport<DO
     }
 
     Collection<ActorSelection> getListenerActors() {
-        return Collections.unmodifiableCollection(listenerActors);
+        return new ArrayList<>(listenerActors);
     }
 
     @Override

diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/DataTreeCohortActorRegistry.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/DataTreeCohortActorRegistry.java
index f79c0475bb73afc025545704ac590ab058961723..c367f98f5aedd1b8282e96bb3e1549aa8023a30a 100644 (file)
--- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/DataTreeCohortActorRegistry.java
+++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/DataTreeCohortActorRegistry.java
@@ -15,7 +15,6 @@ import akka.util.Timeout;
 import com.google.common.base.Preconditions;
 import java.util.ArrayList;
 import java.util.Collection;
-import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -50,7 +49,7 @@ class DataTreeCohortActorRegistry extends AbstractRegistrationTree<ActorRef> {
     private final Map<ActorRef, RegistrationTreeNode<ActorRef>> cohortToNode = new HashMap<>();
 
     Collection<ActorRef> getCohortActors() {
-        return Collections.unmodifiableCollection(cohortToNode.keySet());
+        return new ArrayList<>(cohortToNode.keySet());
     }
 
     @SuppressWarnings("checkstyle:IllegalCatch")