commit f14ba9d7a13eedbb5a9195e87a6e8f9417de3369
author Arnoud Glimmerveen <arnoud@glimmerveen.org> Sat Nov 09 23:33:34 2019 +0100
committer Christian Schneider <chris@die-schneider.net> Sat Nov 09 23:33:33 2019 +0100
tree a58041a28f1c01cc64363b8f95db0443d164def1
parent 9aeafe825a646fcf753346c548d409e914a53fe0

ARIES-1941 Addressed synchronization inconsistency in (using) MultiMap (#31)

* ARIES-1941 Addressed synchronization inconsistency in (using) MultiMap

Changed the approach to MultiMap to have all public method thread-safe: using synchronized keyword *and* returning read-only *copies* when a Set is to be returned
Changed the one case where a defensive copy was made. Note that this old approach was not thread-safe, as the copy-constructors' iterator could be 'tripped' by a concurrent modification.

* With the change to regular HashMap, a defensive copy of keySet is needed.

topology-manager/src/main/java/org/apache/aries/rsa/topologymanager/importer/MultiMap.java

diff --git a/topology-manager/src/main/java/org/apache/aries/rsa/topologymanager/importer/MultiMap.java b/topology-manager/src/main/java/org/apache/aries/rsa/topologymanager/importer/MultiMap.java
index eac5f40..b0f8dbe 100644
--- a/topology-manager/src/main/java/org/apache/aries/rsa/topologymanager/importer/MultiMap.java
+++ b/topology-manager/src/main/java/org/apache/aries/rsa/topologymanager/importer/MultiMap.java
@@ -19,10 +19,10 @@
 package org.apache.aries.rsa.topologymanager.importer;
 
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.HashSet;
-import java.util.Map;
+import java.util.Map;;
 import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
 
 /**
  * Minimal implementation of a synchronized map
@@ -32,7 +32,7 @@
     private Map<String, Set<T>> map;
     
     public MultiMap() {
-        map = new ConcurrentHashMap<>();
+        map = new HashMap<>();
     }
     
     public synchronized void put(String key, T value) {
@@ -45,7 +45,11 @@
     }
     
     public synchronized Set<T> get(String key) {
-        return map.getOrDefault(key, Collections.<T>emptySet());
+        if (map.containsKey(key)) {
+            return Collections.unmodifiableSet(new HashSet<>(map.get(key)));
+        } else {
+            return Collections.<T>emptySet();
+        }
     }
 
     public synchronized void remove(String key, T value) {
@@ -57,12 +61,11 @@
     }
 
     public synchronized Set<String> keySet() {
-        return map.keySet();
+        return Collections.unmodifiableSet(new HashSet<>(map.keySet()));
     }
 
-    public void remove(T toRemove) {
-        Set<String> keys = new HashSet<>(map.keySet());
-        for (String key : keys) {
+    public synchronized void remove(T toRemove) {
+        for (String key : map.keySet()) {
             remove(key, toRemove);
         }
     }

topology-manager/src/main/java/org/apache/aries/rsa/topologymanager/importer/TopologyManagerImport.java

diff --git a/topology-manager/src/main/java/org/apache/aries/rsa/topologymanager/importer/TopologyManagerImport.java b/topology-manager/src/main/java/org/apache/aries/rsa/topologymanager/importer/TopologyManagerImport.java
index cbe8d5e..43c0bd5 100644
--- a/topology-manager/src/main/java/org/apache/aries/rsa/topologymanager/importer/TopologyManagerImport.java
+++ b/topology-manager/src/main/java/org/apache/aries/rsa/topologymanager/importer/TopologyManagerImport.java
@@ -176,7 +176,7 @@
     }
 
     private void unImportForGoneEndpoints(String filter) {
-        Set<ImportRegistration> importRegistrations = new HashSet<>(importedServices.get(filter));
+        Set<ImportRegistration> importRegistrations = importedServices.get(filter);
         Set<EndpointDescription> endpoints = importPossibilities.get(filter);
         for (ImportRegistration ir : importRegistrations) {
             EndpointDescription endpoint = ir.getImportReference().getImportedEndpoint();