Skip to content

Commit

Permalink
Fix deadlock in AdapterManager
Browse files Browse the repository at this point in the history
currently AdapterManager uses two kinds of synchronizations:

1) synchronized methods on the adapter manger
2) concurrent maps with computeIfAbsent

if can now happen that one thread is currently inside an computeIfAbsent
and calls a synchronized method, while another thread tries to flush the
cache. This then leads to a deadlock as one thread currently holds the
map lock while the other holds the class lock but both are required to
complete.

This now replaces all synchronized AdapterManager methods by non
synchronized but concurrent aware variants as these are already using
thread safe datastructures only. The lockup of a map for adapters is
replaced by an indirection of AdapterLookup that can be constructed lock
free and therefore no further blocks / requires the map lock while
calling other AdapterManager methods.
  • Loading branch information
laeubi committed Apr 20, 2024
1 parent a29e098 commit 49be75d
Showing 1 changed file with 44 additions and 22 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ public final class AdapterManager implements IAdapterManager {
* note</b>: always use the compute methods to update the map and make sure the
* values (inner map) are never modified but replaced if necessary.
*/
private final ConcurrentMap<String, Map<String, List<IAdapterFactory>>> adapterLookup;
private final ConcurrentMap<String, AdapterLookup> adapterLookup;

/**
* Cache of classes for a given type name. Avoids too many loadClass calls.
Expand All @@ -77,7 +77,7 @@ public final class AdapterManager implements IAdapterManager {
* the adaptable class that the factory provides adapters for. Value is a
* <code>List</code> of <code>IAdapterFactory</code>.
*/
private final Map<String, List<IAdapterFactory>> factories;
private final ConcurrentMap<String, List<IAdapterFactory>> factories;

private final Queue<IAdapterManagerProvider> lazyFactoryProviders;

Expand Down Expand Up @@ -173,14 +173,8 @@ public String[] computeAdapterTypes(Class<? extends Object> adaptable) {
*/
private Map<String, List<IAdapterFactory>> getFactories(Class<? extends Object> adaptable) {
// cache reference to lookup to protect against concurrent flush
return adapterLookup.computeIfAbsent(adaptable.getName(), adaptableType -> {
// calculate adapters for the class
Map<String, List<IAdapterFactory>> table = new HashMap<>(4);
for (Class<?> cl : computeClassOrder(adaptable)) {
addFactoriesFor(cl.getName(), table);
}
return Collections.unmodifiableMap(table);
});
return adapterLookup.computeIfAbsent(adaptable.getName(), adaptableType -> new AdapterLookup(adaptable, this))
.getMap();
}

/**
Expand Down Expand Up @@ -241,7 +235,7 @@ private static void computeInterfaceOrder(Class<?>[] interfaces, Collection<Clas
* smart and remove only those entries affected.
* </p>
*/
public synchronized void flushLookup() {
public void flushLookup() {
adapterLookup.clear();
classLookup.clear();
classSearchOrderLookup.clear();
Expand Down Expand Up @@ -346,7 +340,7 @@ public Object loadAdapter(Object adaptable, String adapterTypeName) {
* @see IAdapterManager#registerAdapters
*/
@Override
public synchronized void registerAdapters(IAdapterFactory factory, Class<?> adaptable) {
public void registerAdapters(IAdapterFactory factory, Class<?> adaptable) {
registerFactory(factory, adaptable.getName());
flushLookup();
}
Expand All @@ -362,29 +356,33 @@ public void registerFactory(IAdapterFactory factory, String adaptableType) {
* @see IAdapterManager#unregisterAdapters
*/
@Override
public synchronized void unregisterAdapters(IAdapterFactory factory) {
for (List<IAdapterFactory> list : factories.values())
list.remove(factory);
flushLookup();
public void unregisterAdapters(IAdapterFactory factory) {
for (List<IAdapterFactory> list : factories.values()) {
if (list.remove(factory)) {
flushLookup();
}
}
}

/*
* @see IAdapterManager#unregisterAdapters
*/
@Override
public synchronized void unregisterAdapters(IAdapterFactory factory, Class<?> adaptable) {
public void unregisterAdapters(IAdapterFactory factory, Class<?> adaptable) {
List<IAdapterFactory> factoryList = factories.get(adaptable.getName());
if (factoryList == null)
if (factoryList == null || factoryList.isEmpty()) {
return;
factoryList.remove(factory);
flushLookup();
}
if (factoryList.remove(factory)) {
flushLookup();
}
}

/*
* Shuts down the adapter manager by removing all factories and removing the
* registry change listener. Should only be invoked during platform shutdown.
*/
public synchronized void unregisterAllAdapters() {
public void unregisterAllAdapters() {
lazyFactoryProviders.clear();
factories.clear();
flushLookup();
Expand All @@ -409,7 +407,7 @@ public Map<String, List<IAdapterFactory>> getFactories() {
}

/**
* Try to laod the given factory according to the force parameter
* Try to load the given factory according to the force parameter
*
* @param factory the factory to load
* @param force if loading should be forced
Expand All @@ -421,4 +419,28 @@ private static Optional<IAdapterFactory> loadFactory(IAdapterFactory factory, bo
}
return Optional.ofNullable(factory);
}

private static final class AdapterLookup {

private Class<?> adaptable;
private AdapterManager manager;
private Map<String, List<IAdapterFactory>> map;

AdapterLookup(Class<?> adaptable, AdapterManager manager) {
this.adaptable = adaptable;
this.manager = manager;
}

synchronized Map<String, List<IAdapterFactory>> getMap() {
if (map == null) {
// calculate adapters for the class
Map<String, List<IAdapterFactory>> table = new HashMap<>(4);
for (Class<?> cl : manager.computeClassOrder(adaptable)) {
manager.addFactoriesFor(cl.getName(), table);
}
map = Collections.unmodifiableMap(table);
}
return map;
}
}
}

0 comments on commit 49be75d

Please sign in to comment.