SLING-9631 : Avoid read/write lock when processing requests
diff --git a/src/main/java/org/apache/sling/auth/core/impl/PathBasedHolderCache.java b/src/main/java/org/apache/sling/auth/core/impl/PathBasedHolderCache.java
index 24dcf5a..e166fe6 100644
--- a/src/main/java/org/apache/sling/auth/core/impl/PathBasedHolderCache.java
+++ b/src/main/java/org/apache/sling/auth/core/impl/PathBasedHolderCache.java
@@ -20,124 +20,94 @@
import java.util.ArrayList;
import java.util.Collection;
-import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.SortedSet;
import java.util.TreeSet;
-import java.util.concurrent.locks.ReentrantReadWriteLock;
+import java.util.concurrent.ConcurrentHashMap;
import javax.servlet.http.HttpServletRequest;
public class PathBasedHolderCache<Type extends PathBasedHolder> {
- private final Map<String, Map<String, SortedSet<Type>>> cache = new HashMap<String, Map<String, SortedSet<Type>>>();
-
- /** Read/write lock to synchronize the cache access. */
- private final ReentrantReadWriteLock rwLock = new ReentrantReadWriteLock();
+ /**
+ * The cache is a concurrent map of concurrent maps.
+ * As the final value, the sorted set is replaced on each change, reading of the
+ * cache does not need to be synchronized. Updating of the cache is synchronized.
+ */
+ private final Map<String, Map<String, SortedSet<Type>>> cache = new ConcurrentHashMap<>();
public void clear() {
- this.rwLock.writeLock().lock();
- try {
- cache.clear();
- } finally {
- this.rwLock.writeLock().unlock();
- }
+ cache.clear();
}
- public void addHolder(final Type holder) {
- this.rwLock.writeLock().lock();
- try {
+ public synchronized void addHolder(final Type holder) {
+ final Map<String, SortedSet<Type>> byHostMap = cache.computeIfAbsent(holder.protocol, protocol -> new ConcurrentHashMap<>());
- Map<String, SortedSet<Type>> byHostMap = cache.get(holder.protocol);
- if (byHostMap == null) {
- byHostMap = new HashMap<String, SortedSet<Type>>();
- cache.put(holder.protocol, byHostMap);
- }
-
- final SortedSet<Type> byPathSet = new TreeSet<Type>();
-
- // preset with current list
- final SortedSet<Type> currentPathSet = byHostMap.get(holder.host);
- if (currentPathSet != null) {
- byPathSet.addAll(currentPathSet);
- }
-
- // add the new holder
- byPathSet.add(holder);
-
- // replace old set with new set
- byHostMap.put(holder.host, byPathSet);
- } finally {
- this.rwLock.writeLock().unlock();
+ // preset with current list
+ final SortedSet<Type> currentPathSet = byHostMap.get(holder.host);
+ final SortedSet<Type> byPathSet = new TreeSet<Type>();
+ if (currentPathSet != null) {
+ byPathSet.addAll(currentPathSet);
}
+
+ // add the new holder
+ byPathSet.add(holder);
+
+ // replace old set with new set
+ byHostMap.put(holder.host, byPathSet);
}
- public void removeHolder(final Type holder) {
- this.rwLock.writeLock().lock();
- try {
- final Map<String, SortedSet<Type>> byHostMap = cache.get(holder.protocol);
- if (byHostMap != null) {
- final SortedSet<Type> byPathSet = byHostMap.get(holder.host);
- if (byPathSet != null) {
+ public synchronized void removeHolder(final Type holder) {
+ final Map<String, SortedSet<Type>> byHostMap = cache.get(holder.protocol);
+ if (byHostMap != null) {
+ final SortedSet<Type> byPathSet = byHostMap.get(holder.host);
+ if (byPathSet != null) {
- // create a new set without the removed holder
- final SortedSet<Type> set = new TreeSet<Type>();
- set.addAll(byPathSet);
- set.remove(holder);
+ // create a new set without the removed holder
+ final SortedSet<Type> set = new TreeSet<Type>();
+ set.addAll(byPathSet);
+ set.remove(holder);
- // replace the old set with the new one (or remove if empty)
- if (set.isEmpty()) {
- byHostMap.remove(holder.host);
- } else {
- byHostMap.put(holder.host, set);
- }
+ // replace the old set with the new one (or remove if empty)
+ if (set.isEmpty()) {
+ byHostMap.remove(holder.host);
+ } else {
+ byHostMap.put(holder.host, set);
}
}
- } finally {
- this.rwLock.writeLock().unlock();
}
}
public Collection<Type>[] findApplicableHolders(final HttpServletRequest request) {
- this.rwLock.readLock().lock();
- try {
- final String hostname = request.getServerName()
- + (request.getServerPort() != 80 && request.getServerPort() != 443
- ? ":" + request.getServerPort()
- : "");
+ final String hostname = request.getServerName()
+ + (request.getServerPort() != 80 && request.getServerPort() != 443
+ ? ":" + request.getServerPort()
+ : "");
- @SuppressWarnings("unchecked")
- final SortedSet<Type>[] result = new SortedSet[4];
+ @SuppressWarnings("unchecked")
+ final SortedSet<Type>[] result = new SortedSet[4];
- final Map<String, SortedSet<Type>> byHostMap = cache.get(request.getScheme());
- if ( byHostMap != null ) {
- result[0] = byHostMap.get(hostname);
- result[1] = byHostMap.get("");
- }
- final Map<String, SortedSet<Type>> defaultByHostMap = cache.get("");
- if ( defaultByHostMap != null ) {
- result[2] = defaultByHostMap.get(hostname);
- result[3] = defaultByHostMap.get("");
- }
- return result;
- } finally {
- this.rwLock.readLock().unlock();
+ final Map<String, SortedSet<Type>> byHostMap = cache.get(request.getScheme());
+ if ( byHostMap != null ) {
+ result[0] = byHostMap.get(hostname);
+ result[1] = byHostMap.get("");
}
+ final Map<String, SortedSet<Type>> defaultByHostMap = cache.get("");
+ if ( defaultByHostMap != null ) {
+ result[2] = defaultByHostMap.get(hostname);
+ result[3] = defaultByHostMap.get("");
+ }
+ return result;
}
public List<Type> getHolders() {
- this.rwLock.readLock().lock();
- try {
- final List<Type> result = new ArrayList<Type>();
- for (Map<String, SortedSet<Type>> byHostEntry : cache.values()) {
- for (SortedSet<Type> holderSet : byHostEntry.values()) {
- result.addAll(holderSet);
- }
+ final List<Type> result = new ArrayList<Type>();
+ for (Map<String, SortedSet<Type>> byHostEntry : cache.values()) {
+ for (SortedSet<Type> holderSet : byHostEntry.values()) {
+ result.addAll(holderSet);
}
- return result;
- } finally {
- this.rwLock.readLock().unlock();
}
+ return result;
}
}
diff --git a/src/main/java/org/apache/sling/auth/core/impl/SlingAuthenticator.java b/src/main/java/org/apache/sling/auth/core/impl/SlingAuthenticator.java
index 4a25f19..47f1ef9 100644
--- a/src/main/java/org/apache/sling/auth/core/impl/SlingAuthenticator.java
+++ b/src/main/java/org/apache/sling/auth/core/impl/SlingAuthenticator.java
@@ -25,11 +25,11 @@
import java.util.ArrayList;
import java.util.Collection;
import java.util.Dictionary;
-import java.util.HashMap;
import java.util.Hashtable;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
import javax.jcr.SimpleCredentials;
import javax.security.auth.login.AccountLockedException;
@@ -1673,7 +1673,7 @@
private final PathBasedHolderCache<AbstractAuthenticationHandlerHolder> authHandlerCache;
- private final HashMap<Object, AbstractAuthenticationHandlerHolder[]> handlerMap = new HashMap<Object, AbstractAuthenticationHandlerHolder[]>();
+ private final Map<Object, AbstractAuthenticationHandlerHolder[]> handlerMap = new ConcurrentHashMap<>();
AuthenticationHandlerTracker(
final BundleContext context,
@@ -1740,18 +1740,12 @@
}
// keep a copy of them for unregistration later
- synchronized (handlerMap) {
- handlerMap.put(ref.getProperty(Constants.SERVICE_ID),
- holders);
- }
+ handlerMap.put(ref.getProperty(Constants.SERVICE_ID), holders);
}
}
private void unbindAuthHandler(ServiceReference ref) {
- final AbstractAuthenticationHandlerHolder[] holders;
- synchronized (handlerMap) {
- holders = handlerMap.remove(ref.getProperty(Constants.SERVICE_ID));
- }
+ final AbstractAuthenticationHandlerHolder[] holders = handlerMap.remove(ref.getProperty(Constants.SERVICE_ID));
if (holders != null) {
for (AbstractAuthenticationHandlerHolder holder : holders) {
