ContributionContextTypeRegistry is not thread-safe

[trancexpress]

In our automated tests, we see such logged errors:

!ENTRY org.eclipse.egit.ui 4 0 2026-05-21 01:52:19.697
!MESSAGE Cannot register git support for Java templates
!STACK 0
java.util.ConcurrentModificationException
	at java.base/java.util.LinkedHashMap$LinkedHashIterator.nextNode(LinkedHashMap.java:1024)
	at java.base/java.util.LinkedHashMap$LinkedValueIterator.next(LinkedHashMap.java:1053)
	at org.eclipse.egit.ui.internal.StartEventListener.lambda$2(StartEventListener.java:108)
	at org.eclipse.egit.ui.internal.StartEventListener.lambda$0(StartEventListener.java:70)
	at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:40)
	at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:132)
	at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:5084)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:4549)
	at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$5.run(PartRenderingEngine.java:1147)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:339)
	at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.run(PartRenderingEngine.java:1038)
	at org.eclipse.e4.ui.internal.workbench.E4Workbench.createAndRunUI(E4Workbench.java:153)
	at org.eclipse.ui.internal.Workbench.lambda$3(Workbench.java:677)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:339)
	at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:583)
	at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:173)

We started seeing the errors after activating the org.eclipse.jdt.ls.core early in our tests life-cycle.

JDT LS has initialization code that uses:

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/preferences/PreferenceManager.java:                ContextTypeRegistry registry = JavaManipulation.getCodeTemplateContextRegistry();

EGit likewise, as seen in the stack trace above.

Since multiple clients want to initialize templates during their respective bundle activation, we need guidance on what needs to change in the respective bundles, or thread-safety in ContributionContextTypeRegistry.

[merks]

I'm not sure the above represents a thread safety problem. It appears to be running in the UI thread. Is something else running in a non-UI thread? What's actually involved here? Might it be the case that a listener is being added as a result of a notification to a listener?

I don't think you can make an iterator thread safe from outside the logic in EGit itself:

I think we need to understand better what's happening concurrently during the loop, which might well not be something from another thread.

[trancexpress]

As mentioned in the description, see: https://github.com/eclipse-jdtls/eclipse.jdt.ls/blob/main/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/preferences/PreferenceManager.java

It has this code:

		ContextTypeRegistry registry = JavaManipulation.getCodeTemplateContextRegistry();
		if (registry == null) registry = new ContextTypeRegistry();
		// Register standard context types from JDT
		CodeTemplateContextType.registerContextTypes(registry);
		// Register additional context types
		for (String contextTypeId : List.of(
				CodeTemplatePreferences.CLASSSNIPPET_CONTEXTTYPE,
				CodeTemplatePreferences.INTERFACESNIPPET_CONTEXTTYPE,
				CodeTemplatePreferences.RECORDSNIPPET_CONTEXTTYPE)) {
			CodeTemplateContextType contextType = new CodeTemplateContextType(contextTypeId);
			contextType.addResolver(new CodeTemplateVariableResolver(CodeTemplateContextType.FILE_COMMENT, JavaManipulationMessages.CodeTemplateContextType_variable_description_filecomment));
			registry.addContextType(contextType);
		}

		// These should be upstreamed into CodeTemplateContextType & GlobalVariables
		TemplateContextType tmp = registry.getContextType(CodeTemplateContextType.TYPECOMMENT_CONTEXTTYPE);
		tmp.addResolver(new CodeTemplatePreferences.Month());
		tmp.addResolver(new CodeTemplatePreferences.ShortMonth());
		tmp.addResolver(new CodeTemplatePreferences.Day());
		tmp.addResolver(new CodeTemplatePreferences.Hour());
		tmp.addResolver(new CodeTemplatePreferences.Minute());

This code runs in:

org.eclipse.jdt.ls.core.internal.JavaLanguageServerPlugin.start(BundleContext)

I.e. org.eclipse.text.templates.ContextTypeRegistry.fContextTypes is both iterated over (from EGit UI thread call) and is changed (from JDT LS during bundle activation). Or something similar, I've not debugged as its a sporadic CME.

I don't think you can make an iterator thread safe from outside the logic in EGit itself:

I'm not sure either what we are doing here. For now we'll synchronize in our tests. Hopefully this is not an issue that occurs in production...

[trancexpress]

For the CME what we can probably do is add more API, a method that returns a copy of the underlying collection. Then synchronization can be done on the new method and the modification methods. The iterator will remain thread-unsafe, EGit will have to use the new method.

This doesn't solve the problem of adding more context types after the EGit initialization code ran, that EGit will not add its resolver to. Maybe we need API for a listener mechanism. Whenever a new type is added, the listener has a chance to add a resolver to it...

[merks]

It seems quite a tricky problem that clients need to do something differently and even new API. Better is maybe that jdt.ls do the work on the UI thread via an async exec. Less work and easily accomplished with no API changes.

[trancexpress]

Normally such code is denoted with something in the JavaDoc, e.g. in FontRegistry there is:

	 * <p>
	 * This method should only be called from the UI thread. If you are not on the UI
	 * thread then wrap the call with a
	 * <code>PlatformUI.getWorkbench().getDisplay().synchExec()</code> in order to
	 * guarantee the correct result. Failure to do this may result in an {@link
	 * SWTException} being thrown.
	 * </p>

I see no such JavaDoc in ContributionContextTypeRegistry. We can of course set a PR for JDT LS / patch it on our end... @iloveeclipse this would be simplest for us?

[merks]

I’m on a phone so can’t confirm anything. Taking the path of least resistance would seem best. Additional docs would be great too of course.

[iloveeclipse]

We can of course set a PR for JDT LS / patch it on our end.

Jdt.ls shouldn't need UI thread at all, the code in the registry should be fixed.

[merks]

The method org.eclipse.text.templates.ContextTypeRegistry.contextTypes() could return a copy of the list.

	/**
	 * Returns an iterator over all registered context types.
	 *
	 * @return an iterator over all registered context types
	 */
	public Iterator<TemplateContextType> contextTypes() {
		return fContextTypes.values().iterator();
	}

The method's Javadoc does not say much (anything) about the nature of the iterator, i.e., is remove allowed?

The other two methods are not thread safe either, but making them so doesn't look like rocket science...

[iloveeclipse]

The method's Javadoc does not say much (anything) about the nature of the iterator, i.e., is remove allowed?

It is really horrible state. Given the fact it exists so long, good chances people could mis-use that.

The other two methods are not thread safe either

I personally would prefer to check whether we can make this registry thread safe and switch to the "copy" iterator (working on a snapshot). If we do it in M1, we could check whether there are "bad boys" mis-using iterator.

[merks]

It's just a wrapper around a linked hash map, so making it thread safe is not difficult. But you can't make a thread safe iterator (I don't think) so some behavior change would be necessary there, i.e., iterate over a copy as you suggest seems reasonable. Given there is no remove method in the API, one probably should not expect that remove is possible via the iterator. Changing in M1 seems very reasonable...

[trancexpress]

Lets discuss here: #4023

[iloveeclipse]

Lets discuss here: #4023

This is exactly what I had in mind. Let try it for M1.