Skip to content

Fix InitializeConstructors race condition#371

Merged
myieye merged 1 commit intomasterfrom
fix-InitializeConstructors-race-condition
Mar 24, 2026
Merged

Fix InitializeConstructors race condition#371
myieye merged 1 commit intomasterfrom
fix-InitializeConstructors-race-condition

Conversation

@myieye
Copy link
Contributor

@myieye myieye commented Mar 24, 2026

We periodically run into exceptions like this in CI:

-------- System.ArgumentException : An item with the same key has already been added. Key: CmFilter
  Stack Trace:
     at CommonServiceLocator.ServiceLocatorImplBase.GetInstance(Type serviceType, String key)
   at CommonServiceLocator.ServiceLocatorImplBase.GetService(Type serviceType)
   at SIL.LCModel.IocHelpers.GetInstance[TService](IServiceProvider provider)
  
 at SIL.LCModel.LcmCache.CreateCacheInternal(IProjectIdentifier projectId, String userWsIcuLocale, ILcmUI ui, ILcmDirectories dirs, LcmSettings settings, Action`1 doThe, Action`1 initialize)
   at SIL.LCModel.LcmCache.CreateCacheInternal(IProjectIdentifier projectId, String userWsIcuLocale, ILcmUI ui, ILcmDirectories dirs, LcmSettings settings, Action`1 doThe)
   at SIL.LCModel.LcmCache.CreateCacheWithNewBlankLangProj(IProjectIdentifier projectId, String analWsIcuLocale, String vernWsIcuLocale, String userWsIcuLocale, ILcmUI ui, ILcmDirectories dirs, LcmSettings settings)
   at FwDataMiniLcmBridge.Tests.Fixtures.MockFwProjectLoader.NewProject(FwDataProject project, String analysisWs, String vernacularWs) in D:\a\languageforge-lexbox\languageforge-lexbox\backend\FwLite\FwDataMiniLcmBridge.Tests\Fixtures\MockFwProjectLoader.cs:line 27
   at FwLiteProjectSync.Tests.Fixtures.SyncFixture.InitializeAsync() in D:\a\languageforge-lexbox\languageforge-lexbox\backend\FwLite\FwLiteProjectSync.Tests\Fixtures\SyncFixture.cs:line 99
----- Inner Stack Trace -----
   at lambda_method52(Closure, IBuildSession, IContext)
   at StructureMap.Building.BuildPlan.Build(IBuildSession session, IContext context)
   at StructureMap.BuildSession.BuildNewInSession(Type pluginType, Instance instance)
   at StructureMap.BuildSession.BuildNewInOriginalContext(Type pluginType, Instance instance)
   at StructureMap.Pipeline.LifecycleObjectCache.buildWithSession(Type pluginType, Instance instance, IBuildSession session)
   at StructureMap.Pipeline.LifecycleObjectCache.<>c__DisplayClass5_0.<Get>b__0(Int32 _)
   at StructureMap.Pipeline.LazyLifecycleObjectCacheExtensions.<>c__DisplayClass1_1`2.<GetOrAdd>b__1()
   at StructureMap.Pipeline.LazyLifecycleObject`1.CreateValue()
   at StructureMap.Pipeline.LazyLifecycleObject`1.get_Value()
   at StructureMap.Pipeline.LazyLifecycleObjectCacheExtensions.GetOrAdd[TKey,TValue](ConcurrentDictionary`2 cache, TKey key, Func`2 valueFactory)
   at StructureMap.Pipeline.LifecycleObjectCache.Get(Type pluginType, Instance instance, IBuildSession session)
   at StructureMap.BuildSession.ResolveFromLifecycle(Type pluginType, Instance instance)
   at StructureMap.SessionCache.GetObject(Type pluginType, Instance instance, ILifecycle lifecycle)
   at StructureMap.SessionCache.GetDefault(Type pluginType, IPipelineGraph pipelineGraph)
   at StructureMap.BuildSession.GetInstance(Type pluginType)
   at StructureMap.Container.DoGetInstance(Type pluginType)
   at StructureMap.Container.GetInstance(Type pluginType)
   at SIL.LCModel.IOC.StructureMapServiceLocator.DoGetInstance(Type serviceType, String key)
   at CommonServiceLocator.ServiceLocatorImplBase.GetInstance(Type serviceType, String key)
----- Inner Stack Trace -----
   at System.Collections.Generic.Dictionary`2.TryInsert(TKey key, TValue value, InsertionBehavior behavior)
   at System.Collections.Generic.Dictionary`2.Add(TKey key, TValue value)
   at SIL.LCModel.Infrastructure.Impl.CmObjectSurrogate.InitializeConstructors(List`1 cmObjectTypes)
   at SIL.LCModel.Infrastructure.Impl.LcmMetaDataCache..ctor()
   at lambda_method54(Closure, IBuildSession, IContext)

AI wrote this test which effectively reproduced the issue, but I think the fix is logical and straightforward enough that adding this complex test is overkill:

[TestFixture]
public class CmObjectSurrogateTests
{
	private static readonly FieldInfo s_classToConstructorInfoField =
		typeof(CmObjectSurrogate).GetField("s_classToConstructorInfo",
			BindingFlags.Static | BindingFlags.NonPublic);

	/// <summary>
	/// Regression test: concurrent calls to InitializeConstructors should not throw
	/// "An item with the same key has already been added" due to a race condition
	/// on the static s_classToConstructorInfo dictionary.
	/// </summary>
	[Test]
	public void InitializeConstructors_ConcurrentCalls_DoesNotThrow()
	{
		var cmObjectTypes = Assembly.GetAssembly(typeof(LcmCache))
			.GetTypes()
			.Where(t => t.Namespace == "SIL.LCModel.DomainImpl"
				&& t.IsClass && t.GetInterface("ICmObject") != null)
			.ToList();

		// Sort so base types appear first, matching production code
		var cmObjectTypesBaseFirst = new List<Type>();
		while (cmObjectTypes.Count > 0)
		{
			for (var i = cmObjectTypes.Count - 1; i >= 0; i--)
			{
				var baseType = cmObjectTypes[i].BaseType;
				if (baseType == null || !cmObjectTypes.Contains(baseType))
				{
					cmObjectTypesBaseFirst.Add(cmObjectTypes[i]);
					cmObjectTypes.RemoveAt(i);
				}
			}
		}

		const int threadCount = 20;
		const int iterations = 50;

		for (int iteration = 0; iteration < iterations; iteration++)
		{
			// Reset static state via reflection so each iteration forces re-initialization
			s_classToConstructorInfoField.SetValue(null, null);

			var barrier = new Barrier(threadCount);
			var exceptions = new List<Exception>();
			var threads = new Thread[threadCount];

			for (int i = 0; i < threadCount; i++)
			{
				threads[i] = new Thread(() =>
				{
					try
					{
						barrier.SignalAndWait();
						CmObjectSurrogate.InitializeConstructors(cmObjectTypesBaseFirst);
					}
					catch (Exception ex)
					{
						lock (exceptions)
							exceptions.Add(ex);
					}
				});
				threads[i].Start();
			}

			foreach (var t in threads)
				t.Join();

			Assert.That(exceptions, Is.Empty,
				$"Iteration {iteration}: InitializeConstructors threw: {(exceptions.Count > 0 ? exceptions[0].ToString() : "")}");
		}
	}
}

This code is run once per LcmCache, so the tiny performance hit of always acquiring the lock is irrelevant.


This change is Reviewable

@myieye myieye added the 🟩Low Low-priority PR label Mar 24, 2026
@github-actions
Copy link

github-actions bot commented Mar 24, 2026

LCM Tests

    16 files  ±0      16 suites  ±0   3m 6s ⏱️ +11s
 2 858 tests ±0   2 838 ✅ ±0   20 💤 ±0  0 ❌ ±0 
11 380 runs  ±0  11 212 ✅ ±0  168 💤 ±0  0 ❌ ±0 

Results for commit 0dd3041. ± Comparison against base commit b138092.

♻️ This comment has been updated with latest results.

@myieye myieye force-pushed the fix-InitializeConstructors-race-condition branch from 3d89cb5 to 0dd3041 Compare March 24, 2026 13:04
@myieye myieye merged commit c459605 into master Mar 24, 2026
4 checks passed
@myieye myieye deleted the fix-InitializeConstructors-race-condition branch March 24, 2026 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🟩Low Low-priority PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants