Description Olaf Titz 2021-03-26 04:46:11 EDT

This applies to org.eclipse.equinox.common 3.11.0.

In a big RCP based business application, we see frequent errors of this kind:

java.util.ConcurrentModificationException
	at java.base/java.util.ArrayList$Itr.checkForComodification(Unknown Source)
	at java.base/java.util.ArrayList$Itr.next(Unknown Source)
	at org.eclipse.core.internal.runtime.AdapterManager.addFactoriesFor(AdapterManager.java:106)
	at org.eclipse.core.internal.runtime.AdapterManager.getFactories(AdapterManager.java:212)
	at org.eclipse.core.internal.runtime.AdapterManager.getAdapter(AdapterManager.java:296)

called by a class implementing IAdaptable simply as

    public <T> T getAdapter(Class<T> adapterType) {
        return Platform.getAdapterManager().getAdapter(this, adapterType);
    }

This happens on application startup, where the application does a lot of programmatic adapter factory registrations and starts background threads, also doing adapter factory registrations, mixed with getAdapter calls.

The cause of the bug is missing synchronization in certain critical sections in AdapterManager.
AdapterManager has the field

private final HashMap<String, List<IAdapterFactory>> factories;

containing mappings with List<IAdapterFactory> values. This HashMap as well as the contained ArrayList values (filled in by the public void registerFactory(IAdapterFactory factory, String adaptableType) method) are not threadsafe by themselves, so synchronization is already needed for correctness. What is happening here however is a race of the loop in the method

	private void addFactoriesFor(String typeName, Map<String, IAdapterFactory> table) {
		List<IAdapterFactory> factoryList = getFactories().get(typeName);
		if (factoryList == null)
			return;
		for (IAdapterFactory factory : factoryList) {
			// (omitted)
		}
	}

against a concurrent add operation on the same List inside of

	public void registerFactory(IAdapterFactory factory, String adaptableType) {
		List<IAdapterFactory> list = factories.get(adaptableType);
		if (list == null) {
			list = new ArrayList<>(5);
			factories.put(adaptableType, list);
		}
		list.add(factory);
	}

Not all call paths to these methods employ proper synchronization. In particular, the call path

public <T> T getAdapter(Object adaptable, Class<T> adapterType)
-> private Map<String, IAdapterFactory> getFactories(Class<? extends Object> adaptable)
-> private void addFactoriesFor(String typeName, Map<String, IAdapterFactory> table) 

is completely unsynchronized. Any registerFactory call happening while addFactoriesFor is running on the same adaptable type will break the loop in addFactoriesFor.

This (apparently very old) bug in the Eclipse Equinox framework has been severely aggravated by a recent change, which changes the loop from
		for (int i = 0; i < factoryList.size(); ++i) {
			IAdapterFactory factory = factoryList.get(i);
			// (omitted)
		}
to the current
		for (IAdapterFactory factory : factoryList) {
			// (omitted)
		}

While this is seemingly functionally equivalent, the older form has the advantage that an intervening add to the end of the list by a second thread, despite being thread-unsafe on ArrayList, with very high probability will just work. This is because the repeated calls to size will then just give the new size and the repeated calls to get will just get the new elements (and size and get in ArrayList are in fact atomic). The newer form employs a deliberately fail-fast Iterator instead.

To fix this, all methods in AdapterManager accessing the internal data structures have to be properly synchronized. Especially the methods

private Map<String, IAdapterFactory> getFactories(Class<? extends Object> adaptable) and
public void registerFactory(IAdapterFactory factory, String adaptableType)

have to be declared synchronized, but check also all other unsynchronized methods for possible conflicts.

Note that it is not sufficient to wrap the internal data structures in Collections.synchronized... forms.