SLING-2558 : Potential Deadlocks may be caused by AdapterManager
git-svn-id: https://svn.apache.org/repos/asf/sling/trunk@1372417 13f79535-47bb-0310-9956-ffa450edef68
diff --git a/src/main/java/org/apache/sling/adapter/internal/AdapterFactoryDescriptor.java b/src/main/java/org/apache/sling/adapter/internal/AdapterFactoryDescriptor.java
index fe64e9c..efb215a 100644
--- a/src/main/java/org/apache/sling/adapter/internal/AdapterFactoryDescriptor.java
+++ b/src/main/java/org/apache/sling/adapter/internal/AdapterFactoryDescriptor.java
@@ -29,7 +29,7 @@
*/
public class AdapterFactoryDescriptor {
- private AdapterFactory factory;
+ private volatile AdapterFactory factory;
private final String[] adapters;
@@ -48,12 +48,8 @@
public AdapterFactory getFactory() {
if ( factory == null ) {
- synchronized ( this ) {
- if ( factory == null ) {
- factory = (AdapterFactory) context.locateService(
- "AdapterFactory", reference);
- }
- }
+ factory = (AdapterFactory) context.locateService(
+ "AdapterFactory", reference);
}
return factory;
}
diff --git a/src/main/java/org/apache/sling/adapter/internal/AdapterManagerImpl.java b/src/main/java/org/apache/sling/adapter/internal/AdapterManagerImpl.java
index a0f2563..27ce36e 100644
--- a/src/main/java/org/apache/sling/adapter/internal/AdapterManagerImpl.java
+++ b/src/main/java/org/apache/sling/adapter/internal/AdapterManagerImpl.java
@@ -24,10 +24,14 @@
import java.util.ArrayList;
import java.util.Dictionary;
import java.util.HashMap;
+import java.util.HashSet;
import java.util.Hashtable;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
import org.apache.felix.scr.annotations.Component;
import org.apache.felix.scr.annotations.Properties;
@@ -89,18 +93,19 @@
*
* @see AdapterFactoryDescriptorMap
*/
- private Map<String, AdapterFactoryDescriptorMap> factories = new HashMap<String, AdapterFactoryDescriptorMap>();
+ private Map<String, AdapterFactoryDescriptorMap> descriptors = new HashMap<String, AdapterFactoryDescriptorMap>();
/**
- * Matrix of {@link AdapterFactory} instances primarily indexed by the fully
+ * Matrix of {@link AdapterFactoryDescriptor} instances primarily indexed by the fully
* qualified name of the class to be adapted and secondarily indexed by the
* fully qualified name of the class to adapt to (the target class).
* <p>
* This cache is built on demand by calling the
- * {@link #getAdapterFactories(Class)} class. It is removed altogether
+ * {@link #getAdapterFactories(Class)} method. It is cleared
* whenever an adapter factory is registered on unregistered.
*/
- private Map<String, Map<String, AdapterFactory>> factoryCache;
+ private final ConcurrentMap<String, Map<String, AdapterFactoryDescriptor>> factoryCache
+ = new ConcurrentHashMap<String, Map<String, AdapterFactoryDescriptor>>();
/**
* The service tracker for the event admin
@@ -113,15 +118,18 @@
/**
* Returns the adapted <code>adaptable</code> or <code>null</code> if
* the object cannot be adapted.
+ *
+ * @see org.apache.sling.api.adapter.AdapterManager#getAdapter(java.lang.Object, java.lang.Class)
*/
- public <AdapterType> AdapterType getAdapter(Object adaptable,
- Class<AdapterType> type) {
+ public <AdapterType> AdapterType getAdapter(final Object adaptable,
+ final Class<AdapterType> type) {
// get the adapter factories for the type of adaptable object
- Map<String, AdapterFactory> factories = getAdapterFactories(adaptable.getClass());
+ final Map<String, AdapterFactoryDescriptor> factories = getAdapterFactories(adaptable.getClass());
// get the factory for the target type
- AdapterFactory factory = factories.get(type.getName());
+ final AdapterFactoryDescriptor desc = factories.get(type.getName());
+ final AdapterFactory factory = desc == null ? null : desc.getFactory();
// have the factory adapt the adaptable if the factory exists
if (factory != null) {
@@ -139,7 +147,12 @@
// ----------- SCR integration ---------------------------------------------
- protected void activate(ComponentContext context) {
+ /**
+ * Activate the manager.
+ * Bind all already registered factories
+ * @param context Component context
+ */
+ protected void activate(final ComponentContext context) {
this.context = context;
// register all adapter factories bound before activation
@@ -148,7 +161,7 @@
refs = new ArrayList<ServiceReference>(this.boundAdapterFactories);
boundAdapterFactories.clear();
}
- for (ServiceReference reference : refs) {
+ for (final ServiceReference reference : refs) {
registerAdapterFactory(context, reference);
}
@@ -157,14 +170,18 @@
}
/**
+ * Deactivate
* @param context Not used
*/
- protected void deactivate(ComponentContext context) {
+ protected void deactivate(final ComponentContext context) {
SyntheticResource.unsetAdapterManager(this);
this.context = null;
}
- protected void bindAdapterFactory(ServiceReference reference) {
+ /**
+ * Bind a new adapter factory.
+ */
+ protected void bindAdapterFactory(final ServiceReference reference) {
boolean create = true;
if (context == null) {
synchronized ( this.boundAdapterFactories ) {
@@ -179,7 +196,10 @@
}
}
- protected void unbindAdapterFactory(ServiceReference reference) {
+ /**
+ * Unbind a adapter factory.
+ */
+ protected void unbindAdapterFactory(final ServiceReference reference) {
unregisterAdapterFactory(reference);
}
@@ -192,7 +212,7 @@
* MODIFIED WITHOUT NOTICE.</em></strong>
*/
Map<String, AdapterFactoryDescriptorMap> getFactories() {
- return factories;
+ return descriptors;
}
/**
@@ -201,7 +221,7 @@
* <strong><em>THIS METHOD IS FOR UNIT TESTING ONLY. IT MAY BE REMOVED OR
* MODIFIED WITHOUT NOTICE.</em></strong>
*/
- Map<String, Map<String, AdapterFactory>> getFactoryCache() {
+ Map<String, Map<String, AdapterFactoryDescriptor>> getFactoryCache() {
return factoryCache;
}
@@ -209,8 +229,8 @@
* Unregisters the {@link AdapterFactory} referred to by the service
* <code>reference</code> from the registry.
*/
- private void registerAdapterFactory(ComponentContext context,
- ServiceReference reference) {
+ private void registerAdapterFactory(final ComponentContext context,
+ final ServiceReference reference) {
final String[] adaptables = PropertiesUtil.toStringArray(reference.getProperty(ADAPTABLE_CLASSES));
final String[] adapters = PropertiesUtil.toStringArray(reference.getProperty(ADAPTER_CLASSES));
@@ -222,19 +242,22 @@
final AdapterFactoryDescriptor factoryDesc = new AdapterFactoryDescriptor(context,
reference, adapters);
- synchronized (factories) {
- for (final String adaptable : adaptables) {
- AdapterFactoryDescriptorMap adfMap = factories.get(adaptable);
+ for (final String adaptable : adaptables) {
+ AdapterFactoryDescriptorMap adfMap = null;
+ synchronized ( this.descriptors ) {
+ adfMap = descriptors.get(adaptable);
if (adfMap == null) {
adfMap = new AdapterFactoryDescriptorMap();
- factories.put(adaptable, adfMap);
+ descriptors.put(adaptable, adfMap);
}
+ }
+ synchronized ( adfMap ) {
adfMap.put(reference, factoryDesc);
}
}
// clear the factory cache to force rebuild on next access
- factoryCache = null;
+ this.factoryCache.clear();
// send event
final EventAdmin localEA = this.eventAdmin;
@@ -251,7 +274,7 @@
* Unregisters the {@link AdapterFactory} referred to by the service
* <code>reference</code> from the registry.
*/
- private void unregisterAdapterFactory(ServiceReference reference) {
+ private void unregisterAdapterFactory(final ServiceReference reference) {
synchronized ( this.boundAdapterFactories ) {
boundAdapterFactories.remove(reference);
}
@@ -264,14 +287,14 @@
}
boolean factoriesModified = false;
- synchronized (factories) {
- for (String adaptable : adaptables) {
- AdapterFactoryDescriptorMap adfMap = factories.get(adaptable);
- if (adfMap != null) {
+ AdapterFactoryDescriptorMap adfMap = null;
+ for (final String adaptable : adaptables) {
+ synchronized ( this.descriptors ) {
+ adfMap = this.descriptors.get(adaptable);
+ }
+ if (adfMap != null) {
+ synchronized ( adfMap ) {
factoriesModified |= (adfMap.remove(reference) != null);
- if (adfMap.isEmpty()) {
- factories.remove(adaptable);
- }
}
}
}
@@ -279,7 +302,7 @@
// only remove cache if some adapter factories have actually been
// removed
if (factoriesModified) {
- factoryCache = null;
+ this.factoryCache.clear();
}
// send event
@@ -294,28 +317,6 @@
}
/**
- * Returns a map of {@link AdapterFactory} instances for the given class to
- * be adapted. The returned map is indexed by the fully qualified name of
- * the target classes (to adapt to) registered.
- *
- * @param clazz The type of the object for which the registered adapter
- * factories are requested
- * @return The map of adapter factories. If there is no adapter factory
- * registered for this type, the returned map is empty.
- */
- private Map<String, AdapterFactory> getAdapterFactories(Class<?> clazz) {
- Map<String, Map<String, AdapterFactory>> cache = factoryCache;
- if (cache == null) {
- cache = new HashMap<String, Map<String, AdapterFactory>>();
- factoryCache = cache;
- }
-
- synchronized (cache) {
- return getAdapterFactories(clazz, cache);
- }
- }
-
- /**
* Returns the map of adapter factories index by adapter (target) class name
* for the given adaptable <code>clazz</code>. If no adapter exists for
* the <code>clazz</code> and empty map is returned.
@@ -327,15 +328,13 @@
* empty if there is no adapter factory for the adaptable
* <code>clazz</code>.
*/
- private Map<String, AdapterFactory> getAdapterFactories(Class<?> clazz,
- Map<String, Map<String, AdapterFactory>> cache) {
-
- String className = clazz.getName();
- Map<String, AdapterFactory> entry = cache.get(className);
+ private Map<String, AdapterFactoryDescriptor> getAdapterFactories(final Class<?> clazz) {
+ final String className = clazz.getName();
+ Map<String, AdapterFactoryDescriptor> entry = this.factoryCache.get(className);
if (entry == null) {
// create entry
- entry = createAdapterFactoryMap(clazz, cache);
- cache.put(className, entry);
+ entry = createAdapterFactoryMap(clazz);
+ this.factoryCache.put(className, entry);
}
return entry;
@@ -350,43 +349,45 @@
*
* @param clazz The adaptable <code>Class</code> for which to build the
* adapter factory map by target class name.
- * @param cache The cache of already defined adapter factory mappings
* @return The map of adapter factories by target class name. The map may be
* empty if there is no adapter factory for the adaptable
* <code>clazz</code>.
*/
- private Map<String, AdapterFactory> createAdapterFactoryMap(Class<?> clazz,
- Map<String, Map<String, AdapterFactory>> cache) {
- Map<String, AdapterFactory> afm = new HashMap<String, AdapterFactory>();
+ private Map<String, AdapterFactoryDescriptor> createAdapterFactoryMap(final Class<?> clazz) {
+ final Map<String, AdapterFactoryDescriptor> afm = new HashMap<String, AdapterFactoryDescriptor>();
// AdapterFactories for this class
- synchronized (factories) {
- AdapterFactoryDescriptorMap afdMap = factories.get(clazz.getName());
- if (afdMap != null) {
- for (AdapterFactoryDescriptor afd : afdMap.values()) {
- String[] adapters = afd.getAdapters();
- for (String adapter : adapters) {
- if (!afm.containsKey(adapter)) {
- final AdapterFactory factory = afd.getFactory();
- if (factory != null) {
- afm.put(adapter, factory);
- }
- }
+ AdapterFactoryDescriptorMap afdMap = null;
+ synchronized ( this.descriptors ) {
+ afdMap = this.descriptors.get(clazz.getName());
+ }
+ if (afdMap != null) {
+ final Set<AdapterFactoryDescriptor> afdSet;
+ synchronized ( afdMap ) {
+ afdSet = new HashSet<AdapterFactoryDescriptor>(afdMap.values());
+ }
+ for (final AdapterFactoryDescriptor afd : afdSet) {
+ final String[] adapters = afd.getAdapters();
+ for (final String adapter : adapters) {
+ // to handle service ranking, we only add if the map does not
+ // have a value for this adapter yet
+ if (!afm.containsKey(adapter)) {
+ afm.put(adapter, afd);
}
}
}
}
// AdapterFactories for the interfaces
- Class<?>[] interfaces = clazz.getInterfaces();
- for (Class<?> iFace : interfaces) {
- copyAdapterFactories(afm, iFace, cache);
+ final Class<?>[] interfaces = clazz.getInterfaces();
+ for (final Class<?> iFace : interfaces) {
+ copyAdapterFactories(afm, iFace);
}
// AdapterFactories for the super class
- Class<?> superClazz = clazz.getSuperclass();
+ final Class<?> superClazz = clazz.getSuperclass();
if (superClazz != null) {
- copyAdapterFactories(afm, superClazz, cache);
+ copyAdapterFactories(afm, superClazz);
}
return afm;
@@ -403,19 +404,16 @@
* replaced.
* @param clazz The adaptable class whose adapter factories are considered
* for adding into <code>dest</code>.
- * @param cache The adapter factory cache providing the adapter factories
- * for <code>clazz</code> to consider for copying into
- * <code>dest</code>.
*/
- private void copyAdapterFactories(Map<String, AdapterFactory> dest,
- Class<?> clazz, Map<String, Map<String, AdapterFactory>> cache) {
+ private void copyAdapterFactories(final Map<String, AdapterFactoryDescriptor> dest,
+ final Class<?> clazz) {
// get the adapter factories for the adaptable clazz
- Map<String, AdapterFactory> scMap = getAdapterFactories(clazz, cache);
+ final Map<String, AdapterFactoryDescriptor> scMap = getAdapterFactories(clazz);
// for each target class copy the entry to dest if dest does
// not contain the target class already
- for (Map.Entry<String, AdapterFactory> entry : scMap.entrySet()) {
+ for (Map.Entry<String, AdapterFactoryDescriptor> entry : scMap.entrySet()) {
if (!dest.containsKey(entry.getKey())) {
dest.put(entry.getKey(), entry.getValue());
}
diff --git a/src/test/java/org/apache/sling/adapter/internal/AdapterManagerTest.java b/src/test/java/org/apache/sling/adapter/internal/AdapterManagerTest.java
index e94a26b..d6fa2e7 100644
--- a/src/test/java/org/apache/sling/adapter/internal/AdapterManagerTest.java
+++ b/src/test/java/org/apache/sling/adapter/internal/AdapterManagerTest.java
@@ -161,7 +161,7 @@
@org.junit.Test public void testUnitialized() {
assertNotNull("AdapterFactoryDescriptors must not be null", am.getFactories());
assertTrue("AdapterFactoryDescriptors must be empty", am.getFactories().isEmpty());
- assertNull("AdapterFactory cache must be null", am.getFactoryCache());
+ assertTrue("AdapterFactory cache must be empty", am.getFactoryCache().isEmpty());
}
@org.junit.Test public void testInitialized() throws Exception {
@@ -169,7 +169,7 @@
assertNotNull("AdapterFactoryDescriptors must not be null", am.getFactories());
assertTrue("AdapterFactoryDescriptors must be empty", am.getFactories().isEmpty());
- assertNull("AdapterFactory cache must be null", am.getFactoryCache());
+ assertTrue("AdapterFactory cache must be empty", am.getFactoryCache().isEmpty());
}
@org.junit.Test public void testBindBeforeActivate() throws Exception {
@@ -179,14 +179,14 @@
// no cache and no factories yet
assertNotNull("AdapterFactoryDescriptors must not be null", am.getFactories());
assertTrue("AdapterFactoryDescriptors must be empty", am.getFactories().isEmpty());
- assertNull("AdapterFactory cache must be null", am.getFactoryCache());
+ assertTrue("AdapterFactory cache must be empty", am.getFactoryCache().isEmpty());
am.activate(this.createComponentContext());
// expect the factory, but cache is empty
assertNotNull("AdapterFactoryDescriptors must not be null", am.getFactories());
assertEquals("AdapterFactoryDescriptors must contain one entry", 1, am.getFactories().size());
- assertNull("AdapterFactory cache must be null", am.getFactoryCache());
+ assertTrue("AdapterFactory cache must be empty", am.getFactoryCache().isEmpty());
}
@org.junit.Test public void testBindAfterActivate() throws Exception {
@@ -195,7 +195,7 @@
// no cache and no factories yet
assertNotNull("AdapterFactoryDescriptors must not be null", am.getFactories());
assertTrue("AdapterFactoryDescriptors must be empty", am.getFactories().isEmpty());
- assertNull("AdapterFactory cache must be null", am.getFactoryCache());
+ assertTrue("AdapterFactory cache must be empty", am.getFactoryCache().isEmpty());
final ServiceReference ref = createServiceReference();
am.bindAdapterFactory(ref);
@@ -203,7 +203,7 @@
// expect the factory, but cache is empty
assertNotNull("AdapterFactoryDescriptors must not be null", am.getFactories());
assertEquals("AdapterFactoryDescriptors must contain one entry", 1, am.getFactories().size());
- assertNull("AdapterFactory cache must be null", am.getFactoryCache());
+ assertTrue("AdapterFactory cache must be empty", am.getFactoryCache().isEmpty());
Map<String, AdapterFactoryDescriptorMap> f = am.getFactories();
AdapterFactoryDescriptorMap afdm = f.get(TestSlingAdaptable.class.getName());
