Improvements to code involving synchronized maps

jackrabbit-api/pom.xml

@@ -26,7 +26,7 @@
   <parent>
     <groupId>org.apache.jackrabbit</groupId>
     <artifactId>jackrabbit-parent</artifactId>
-    <version>2.15.7-SNAPSHOT</version>
+    <version>3.0.0-SNAPSHOT</version>
     <relativePath>../jackrabbit-parent/pom.xml</relativePath>
   </parent>
   <artifactId>jackrabbit-api</artifactId>

jackrabbit-aws-ext/pom.xml

@@ -20,7 +20,7 @@
     <parent>
         <groupId>org.apache.jackrabbit</groupId>
         <artifactId>jackrabbit-parent</artifactId>
-        <version>2.15.7-SNAPSHOT</version>
+        <version>3.0.0-SNAPSHOT</version>
         <relativePath>../jackrabbit-parent/pom.xml</relativePath>
     </parent>
     <artifactId>jackrabbit-aws-ext</artifactId>

jackrabbit-bundle/pom.xml

@@ -26,7 +26,7 @@
   <parent>
     <groupId>org.apache.jackrabbit</groupId>
     <artifactId>jackrabbit-parent</artifactId>
-    <version>2.15.7-SNAPSHOT</version>
+    <version>3.0.0-SNAPSHOT</version>
     <relativePath>../jackrabbit-parent/pom.xml</relativePath>
   </parent>
   <artifactId>jackrabbit-bundle</artifactId>

jackrabbit-core/pom.xml

@@ -26,7 +26,7 @@
   <parent>
     <groupId>org.apache.jackrabbit</groupId>
     <artifactId>jackrabbit-parent</artifactId>
-    <version>2.15.7-SNAPSHOT</version>
+    <version>3.0.0-SNAPSHOT</version>
     <relativePath>../jackrabbit-parent/pom.xml</relativePath>
   </parent>
   <artifactId>jackrabbit-core</artifactId>

jackrabbit-core/src/main/java/org/apache/jackrabbit/core/cache/CacheManager.java

@@ -17,7 +17,10 @@
 package org.apache.jackrabbit.core.cache;
 
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.List;
+import java.util.Map;
+import java.util.Set;
 import java.util.WeakHashMap;
 
 import org.slf4j.Logger;
@@ -50,7 +53,10 @@ public class CacheManager implements CacheAccessListener {
     private static final long DEFAULT_MAX_MEMORY_PER_CACHE = 4 * 1024 * 1024;
 
     /** The set of caches (weakly referenced). */
-    private WeakHashMap<Cache, Object> caches = new WeakHashMap<Cache, Object>();
+    // This is a weak set. Although Java does not have a WeakSet (see
+    // https://stackoverflow.com/questions/4062919/why-does-exist-weakhashmap-but-absent-weakset )
+    // we can emulate a weak set by creating a WeakHashMap with value type Void.
+    private final Map<Cache, Void> caches = Collections.synchronizedMap(new WeakHashMap<Cache, Void>());
 
     /** The default minimum resize interval (in ms). */
     private static final int DEFAULT_MIN_RESIZE_INTERVAL = 1000;
@@ -163,8 +169,11 @@ private void logCacheStats() {
             }
             // JCR-3194 avoid ConcurrentModificationException
             List<Cache> list = new ArrayList<Cache>();
+            Set<Cache> keys = caches.keySet();
+            // must manually synch on caches
+            // https://docs.oracle.com/javase/8/docs/api/java/util/Collections.html#synchronizedMap-java.util.Map-
             synchronized (caches) {
-                list.addAll(caches.keySet());
+                list.addAll(keys);
             }
             for (Cache cache : list) {
                 log.debug(cache.getCacheInfoAsString());
@@ -184,11 +193,15 @@ private void resizeAll() {
         // entries in a weak hash map may disappear any time
         // so can't use size() / keySet() directly
         // only using the iterator guarantees that we don't get null references
+        // see: https://stackoverflow.com/a/46699068/196844
         List<Cache> list = new ArrayList<Cache>();
+        Set<Cache> keys = caches.keySet();
+        // must manually synch on caches
+        // https://docs.oracle.com/javase/8/docs/api/java/util/Collections.html#synchronizedMap-java.util.Map-
         synchronized (caches) {
-            list.addAll(caches.keySet());
+            list.addAll(keys);
         }
-        if (list.size() == 0) {
+        if (list.isEmpty()) {
             // nothing to do
             return;
         }
@@ -256,12 +269,13 @@ private void resizeAll() {
      * Add a new cache to the list.
      * This call does not trigger recalculating the cache sizes.
      *
-     * @param cache the cache to add
+     * @param cache the cache to add, which may not be <code>null</code>.
      */
     public void add(Cache cache) {
-        synchronized (caches) {
-            caches.put(cache, null);
+        if (cache == null) {
+            throw new NullPointerException("cache must not be null.");
         }
+        caches.put(cache, null);
     }
 
     /**
@@ -270,12 +284,13 @@ public void add(Cache cache) {
      * This call does not trigger recalculating the cache sizes.
      *
      * @param cache
-     *            the cache to remove
+     *            the cache to remove, which may not be <code>null</code>.
      */
     public void remove(Cache cache) {
-        synchronized (caches) {
-            caches.remove(cache);
+        if (cache == null) {
+            throw new NullPointerException("cache must not be null.");
         }
+        caches.remove(cache);
     }
 
     /**

jackrabbit-core/src/main/java/org/apache/jackrabbit/core/nodetype/NodeTypeRegistry.java

@@ -40,7 +40,6 @@
 import javax.jcr.nodetype.NoSuchNodeTypeException;
 import javax.jcr.version.OnParentVersionAction;
 
-import org.apache.commons.collections.map.ReferenceMap;
 import org.apache.commons.io.IOUtils;
 import org.apache.jackrabbit.core.cluster.NodeTypeEventChannel;
 import org.apache.jackrabbit.core.cluster.NodeTypeEventListener;
@@ -62,6 +61,7 @@
 import org.slf4j.LoggerFactory;
 
 import EDU.oswego.cs.dl.util.concurrent.ConcurrentReaderHashMap;
+import java.util.WeakHashMap;
 
 /**
  * A <code>NodeTypeRegistry</code> ...
@@ -119,9 +119,11 @@ public class NodeTypeRegistry implements NodeTypeEventListener {
     /**
      * Listeners (weak references)
      */
-    @SuppressWarnings("unchecked")
-    private final Map<NodeTypeRegistryListener, NodeTypeRegistryListener> listeners =
-            Collections.synchronizedMap(new ReferenceMap(ReferenceMap.WEAK, ReferenceMap.WEAK));
+    // This is a weak set. Although Java does not have a WeakSet (see
+    // https://stackoverflow.com/questions/4062919/why-does-exist-weakhashmap-but-absent-weakset )
+    // we can emulate a weak set by creating a WeakHashMap with value type Void.
+    private final Map<NodeTypeRegistryListener, Void> listeners =
+            Collections.synchronizedMap(new WeakHashMap<NodeTypeRegistryListener, Void>());
 
     /**
      * Node type event channel.
@@ -575,20 +577,24 @@ public boolean isBuiltIn(Name nodeTypeName) {
      * Add a <code>NodeTypeRegistryListener</code>
      *
      * @param listener the new listener to be informed on (un)registration
-     *                 of node types
+     *                 of node types. May not be <code>null</code>.
      */
     public void addListener(NodeTypeRegistryListener listener) {
-        if (!listeners.containsKey(listener)) {
-            listeners.put(listener, listener);
+        if (listener == null) {
+            throw new NullPointerException("listener must not be null.");
         }
+        listeners.put(listener, null);
     }
 
     /**
      * Remove a <code>NodeTypeRegistryListener</code>
      *
-     * @param listener an existing listener
+     * @param listener an existing listener, which may not be <code>null</code>.
      */
     public void removeListener(NodeTypeRegistryListener listener) {
+        if (listener == null) {
+            throw new NullPointerException("listener must not be null.");
+        }
         listeners.remove(listener);
     }
 
@@ -1836,13 +1842,27 @@ private static QNodeDefinition createRootNodeDef() {
      * @param ntName node type name
      */
     private void notifyRegistered(Name ntName) {
-        // copy listeners to array to avoid ConcurrentModificationException
-        NodeTypeRegistryListener[] la = listeners.values().toArray(
-                        new NodeTypeRegistryListener[listeners.size()]);
-        for (NodeTypeRegistryListener aLa : la) {
-            if (aLa != null) {
-                aLa.nodeTypeRegistered(ntName);
+        // copy listeners to an array to avoid ConcurrentModificationException
+        //
+        // listeners is a synchronized map backed by a WeakHashMap. To ensure
+        // that we do not wastefully create an array that is too small to store
+        // the NodeTypeRegistryListener instances (which can happen if a listener
+        // is added between the time that size() returns and toArray() begins),
+        // we need to synchronize on listeners for the duration of the size()
+        // and toArray() calls.
+        final NodeTypeRegistryListener[] la;
+        {
+            final Set<NodeTypeRegistryListener> keys = listeners.keySet();
+            synchronized (listeners) {
+                la = keys.toArray(new NodeTypeRegistryListener[keys.size()]);
+            }
+        }
+        for (final NodeTypeRegistryListener l : la) {
+            // See: https://stackoverflow.com/a/46699068/196844
+            if (l == null) {
+                break;
             }
+            l.nodeTypeRegistered(ntName);
         }
     }
 
@@ -1851,28 +1871,56 @@ private void notifyRegistered(Name ntName) {
      * @param ntName node type name
      */
     private void notifyReRegistered(Name ntName) {
-        // copy listeners to array to avoid ConcurrentModificationException
-        NodeTypeRegistryListener[] la = listeners.values().toArray(
-                        new NodeTypeRegistryListener[listeners.size()]);
-        for (NodeTypeRegistryListener aLa : la) {
-            if (aLa != null) {
-                aLa.nodeTypeReRegistered(ntName);
+        // copy listeners to an array to avoid ConcurrentModificationException
+        //
+        // listeners is a synchronized map backed by a WeakHashMap. To ensure
+        // that we do not wastefully create an array that is too small to store
+        // the NodeTypeRegistryListener instances (which can happen if a listener
+        // is added between the time that size() returns and toArray() begins),
+        // we need to synchronize on listeners for the duration of the size()
+        // and toArray() calls.
+        final NodeTypeRegistryListener[] la;
+        {
+            final Set<NodeTypeRegistryListener> keys = listeners.keySet();
+            synchronized (listeners) {
+                la = keys.toArray(new NodeTypeRegistryListener[keys.size()]);
             }
         }
+        for (final NodeTypeRegistryListener l : la) {
+            // See: https://stackoverflow.com/a/46699068/196844
+            if (l == null) {
+                break;
+            }
+            l.nodeTypeReRegistered(ntName);
+        }
     }
 
     /**
-     * Notify the listeners that oone or more node types have been unregistered.
+     * Notify the listeners that one or more node types have been unregistered.
      * @param names node type names
      */
     private void notifyUnregistered(Collection<Name> names) {
-        // copy listeners to array to avoid ConcurrentModificationException
-        NodeTypeRegistryListener[] la = listeners.values().toArray(
-                        new NodeTypeRegistryListener[listeners.size()]);
-        for (NodeTypeRegistryListener aLa : la) {
-            if (aLa != null) {
-                aLa.nodeTypesUnregistered(names);
+        // copy listeners to an array to avoid ConcurrentModificationException
+        //
+        // listeners is a synchronized map backed by a WeakHashMap. To ensure
+        // that we do not wastefully create an array that is too small to store
+        // the NodeTypeRegistryListener instances (which can happen if a listener
+        // is added between the time that size() returns and toArray() begins),
+        // we need to synchronize on listeners for the duration of the size()
+        // and toArray() calls.
+        final NodeTypeRegistryListener[] la;
+        {
+            final Set<NodeTypeRegistryListener> keys = listeners.keySet();
+            synchronized (listeners) {
+                la = keys.toArray(new NodeTypeRegistryListener[keys.size()]);
+            }
+        }
+        for (final NodeTypeRegistryListener l : la) {
+            // See: https://stackoverflow.com/a/46699068/196844
+            if (l == null) {
+                break;
             }
+            l.nodeTypesUnregistered(names);
         }
     }
 }

jackrabbit-core/src/main/java/org/apache/jackrabbit/core/query/lucene/CachingIndexReader.java

@@ -277,14 +277,20 @@ public Document document(int n, FieldSelector fieldSelector)
             throws CorruptIndexException, IOException {
         if (fieldSelector == FieldSelectors.UUID) {
             Document doc;
-            NodeId id = docNumber2id.get(n);
-            if (id == null) {
-                doc = super.document(n, fieldSelector);
-                id = new NodeId(doc.get(FieldNames.UUID));
-                docNumber2id.put(n, id);
-            } else {
-                doc = new Document();
-                doc.add(new IDField(id));
+            // docNumber2id is a synchronized map. Need to make sure that the
+            // conditional put(), which depends on docNumber2id not having a
+            // map entry for key n, is in the same synchronized block as the
+            // call to get().
+            synchronized (docNumber2id) {
+                NodeId id = docNumber2id.get(n);
+                if (id == null) {
+                    doc = super.document(n, fieldSelector);
+                    id = new NodeId(doc.get(FieldNames.UUID));
+                    docNumber2id.put(n, id);
+                } else {
+                    doc = new Document();
+                    doc.add(new IDField(id));
+                }
             }
             return doc;
         } else {