Description Nick Edgar 2007-03-19 14:23:05 EDT

3.3 M5

ObjectContributorManager is accessed from multiple threads (e.g. the UI thread for object contributions, the decoration thread for decorations, and a runtime notification thread for IExtensionChangeHandler notifications), but does not protect its fields.

I noticed this while looking into the decorator optimizations (see bug 177392 and bug 177592).

For example, getAdaptableContributors does:
	if (adaptableLookup != null) {
		adaptableList = (List) adaptableLookup.get(adapterType);
	}
but if flushLookup() gets called between these two statements due to a registry change, the second statement may fail with an NPE.

Or, if both the decoration thread and UI thread try to update the caches simultaneously, it could fail, or they could get corrupted.

I also noticed that AdapterManager tries to protect itself, but not by using the synchronized keyword.  E.g. AdapterManager.getFactories(Class) has:
    private Map getFactories(Class adaptable) {
	//cache reference to lookup to protect against concurrent flush
	Map lookup = adapterLookup;
	if (lookup == null)
		adapterLookup = lookup = Collections.synchronizedMap(new HashMap(30));
	Map table = (Map) lookup.get(adaptable.getName());
	...
Note the extra local var 'lookup' to avoid a second read on 'adapterLookup', and the use of a synchronized map wrapper.

We could use the same approach in ObjectContributorManager, but I'm not sure this is actually a good workaround.  E.g. in a multi-processor, you could end up with multiple maps hanging around for a while.
For more on this, see:
http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html