Thread-safety fixes (#53, #26) and .NET 10 build-warning cleanup (#46)

.github/workflows/build-and-publish.yml

@@ -23,9 +23,6 @@ jobs:
         with:
           submodules: 'true'
 
-      - name: Add msbuild to PATH
-        uses: microsoft/setup-msbuild@v3
-
       - name: Setup .NET
         uses: actions/setup-dotnet@v5
         with:
@@ -41,7 +38,7 @@ jobs:
           echo "APP_VERSION=${{ env.MAJOR }}.${{ env.MINOR }}.${{ env.MINORMINOR }}.${{ env.RUN }}" | Out-File -FilePath $env:GITHUB_ENV -Append
 
       - name: Build
-        run: msbuild OSPSuite.Utility.sln /p:Version=${{env.APP_VERSION}}
+        run: dotnet build OSPSuite.Utility.sln /p:Version=${{env.APP_VERSION}} --configuration Debug
 
       - name : Test
         run: dotnet test OSPSuite.Utility.sln -v normal --no-build --configuration Debug --logger:"html;LogFileName=../testLog_Windows.html"

.github/workflows/build-pr.yml

@@ -24,9 +24,6 @@ jobs:
         with:
           submodules: 'true'
 
-      - name: Add msbuild to PATH
-        uses: microsoft/setup-msbuild@v3
-
       - name: Setup .NET
         uses: actions/setup-dotnet@v5
         with:
@@ -42,7 +39,7 @@ jobs:
           echo "APP_VERSION=${{ env.MAJOR }}.${{ env.MINOR }}.${{ env.MINORMINOR }}.${{ env.RUN }}" | Out-File -FilePath $env:GITHUB_ENV -Append
 
       - name: Build
-        run: msbuild OSPSuite.Utility.sln /p:Version=${{env.APP_VERSION}}
+        run: dotnet build OSPSuite.Utility.sln /p:Version=${{env.APP_VERSION}} --configuration Debug
 
       - name : Test
         run: dotnet test OSPSuite.Utility.sln -v normal --no-build --configuration Debug --logger:"html;LogFileName=../testLog_Windows.html"

src/OSPSuite.Utility/BuilderRepository.cs

@@ -25,7 +25,8 @@ public abstract class BuilderRepository<TBuilder> : IBuilderRepository<TBuilder>
       private readonly ITypeSimplifier _typeSimplifier;
       private List<TBuilder> _allBuilders;
       private readonly ICache<Type, TBuilder> _allBuilderCache = new Cache<Type, TBuilder>();
-      private bool _isInitialized;
+      private readonly object _locker = new object();
+      private volatile bool _isInitialized;
 
       protected BuilderRepository(IContainer container, Type genericBuilderType)
       {
@@ -37,35 +38,41 @@ protected BuilderRepository(IContainer container, Type genericBuilderType)
       public TBuilder BuilderFor(Type type)
       {
          Start();
-         if (_allBuilderCache.Contains(type))
-            return _allBuilderCache[type];
-
-         var allBuilderForType = _allBuilders.Where(b => b.IsSatisfiedBy(type)).ToList();
 
-         //No Builder found. Return null
-         if (allBuilderForType.Count == 0)
-            return null;
-
-         if (allBuilderForType.Count == 1)
-            _allBuilderCache.Add(type, allBuilderForType[0]);
-         else
+         // Resolve and cache under the lock: BuilderFor can be called concurrently (e.g. parallel
+         // qualification export), and the cache Contains-then-Add below is otherwise a check-then-act
+         // race where two threads Add the same key -> "An item with the key '...' has already been added"
+         lock (_locker)
          {
-            //more than one implementation? try to find the one that matches the best the given type
-            var allGenericTypes = allBuilderForType.SelectMany(t => t.GetDeclaredTypesForGeneric(_genericBuilderType)).ToList();
+            if (_allBuilderCache.Contains(type))
+               return _allBuilderCache[type];
 
-            //one builder implements two ITEXBuilder<> does not make sens
-            if (allGenericTypes.Count != allBuilderForType.Count)
-               throw new InvalidOperationException($"Cannot resolve the Builder for type '{type}'. It seems that one builder implements more than one generic interface");
+            var allBuilderForType = _allBuilders.Where(b => b.IsSatisfiedBy(type)).ToList();
 
-            //finds the one that matches the most the implementation
-            var simplifiedImplementation = _typeSimplifier.Simplify(allGenericTypes).ToList();
-            if (simplifiedImplementation.Count == 1)
-               _allBuilderCache.Add(type, allBuilderForType.ElementAt(allGenericTypes.IndexOf(simplifiedImplementation[0])));
-            else
+            //No Builder found. Return null
+            if (allBuilderForType.Count == 0)
                return null;
-         }
 
-         return _allBuilderCache[type];
+            if (allBuilderForType.Count == 1)
+               _allBuilderCache.Add(type, allBuilderForType[0]);
+            else
+            {
+               //more than one implementation? try to find the one that matches the best the given type
+               var allGenericTypes = allBuilderForType.SelectMany(t => t.GetDeclaredTypesForGeneric(_genericBuilderType)).ToList();
+
+               if (allGenericTypes.Count != allBuilderForType.Count)
+                  throw new InvalidOperationException($"Cannot resolve the Builder for type '{type}'. It seems that one builder implements more than one generic interface");
+
+               //finds the one that matches the most the implementation
+               var simplifiedImplementation = _typeSimplifier.Simplify(allGenericTypes).ToList();
+               if (simplifiedImplementation.Count == 1)
+                  _allBuilderCache.Add(type, allBuilderForType.ElementAt(allGenericTypes.IndexOf(simplifiedImplementation[0])));
+               else
+                  return null;
+            }
+
+            return _allBuilderCache[type];
+         }
       }
 
       public TBuilder BuilderFor(object objectToBuild)
@@ -76,8 +83,12 @@ public TBuilder BuilderFor(object objectToBuild)
       public void Start()
       {
          if (_isInitialized) return;
-         _allBuilders = _container.ResolveAll<TBuilder>().ToList();
-         _isInitialized = true;
+         lock (_locker)
+         {
+            if (_isInitialized) return;
+            _allBuilders = _container.ResolveAll<TBuilder>().ToList();
+            _isInitialized = true;
+         }
       }
    }
 }

src/OSPSuite.Utility/Events/EventPublisher.cs

@@ -22,17 +22,25 @@ public EventPublisher(SynchronizationContext context, IExceptionManager exceptio
 
       public void PublishEvent<T>(T eventToPublish)
       {
-         //Before publishing, prunes the list of listener and remove dead references
-         pruneReferences();
+         List<WeakRef<IListener>> listeners;
+         lock (_listeners)
+         {
+            //Before publishing, prunes the list of listener and remove dead references
+            pruneReferences();
+
+            // Snapshot the listeners while holding the lock so the dispatch loop below
+            // cannot race with a concurrent Add/Remove/prune mutating the same list
+            listeners = _listeners.ToList();
+         }
 
-         // Determine if a Listener handles the message of type T
-         // by trying to cast it
-         foreach (var listener in _listeners.ToList())
+         // Determine if a Listener handles the message of type T by trying to cast it.
+         // Dispatch happens outside the lock so handlers never run while the lock is held.
+         foreach (var listener in listeners)
          {
             var receiver = listener.Target as IListener<T>;
             if (receiver == null) continue;
 
-            // We are using SyncronizationContext to handle moving processing
+            // We are using SynchronizationContext to handle moving processing
             // from a background thread to the UI thread without having 
             // to worry about it in the View or Presenter
             _context.Send(state => _exceptionManager.Execute(() => receiver.Handle(eventToPublish)), null);
@@ -82,18 +90,16 @@ private void doWithinLock(Action action)
          }
       }
 
+      // Caller must hold the lock on _listeners (PublishEvent does).
       private void pruneReferences()
       {
-         doWithinLock(() =>
+         for (var i = _listeners.Count - 1; i >= 0; i--)
          {
-            for (var i = _listeners.Count - 1; i >= 0; i--)
+            if (_listeners[i].Target == null)
             {
-               if (_listeners[i].Target == null)
-               {
-                  _listeners.RemoveAt(i);
-               }
+               _listeners.RemoveAt(i);
             }
-         });
+         }
       }
    }
 }

src/OSPSuite.Utility/Exceptions/OSPSuiteException.cs

@@ -1,5 +1,4 @@
 using System;
-using System.Runtime.Serialization;
 
 namespace OSPSuite.Utility.Exceptions
 {
@@ -19,9 +18,5 @@ public OSPSuiteException(string message) : base(message)
       public OSPSuiteException(string message, Exception innerException) : base(message, innerException)
       {
       }
-
-      protected OSPSuiteException(SerializationInfo info, StreamingContext context) : base(info, context)
-      {
-      }
    }
 }

src/OSPSuite.Utility/FileHelper.cs

@@ -187,17 +187,19 @@ public static bool AreFilesEqual(string fileFullPath1, string fileFullPath2)
       {
          if (!FileExists(fileFullPath1) || !FileExists(fileFullPath2)) return false;
 
-         var hashAlgorithm = HashAlgorithm.Create("MD5");
          byte[] hashFile1;
          byte[] hashFile2;
-         using (var fs = new FileStream(fileFullPath1, FileMode.Open))
+         using (var hashAlgorithm = MD5.Create())
          {
-            hashFile1 = hashAlgorithm.ComputeHash(fs);
-         }
+            using (var fs = new FileStream(fileFullPath1, FileMode.Open))
+            {
+               hashFile1 = hashAlgorithm.ComputeHash(fs);
+            }
 
-         using (var fs = new FileStream(fileFullPath2, FileMode.Open))
-         {
-            hashFile2 = hashAlgorithm.ComputeHash(fs);
+            using (var fs = new FileStream(fileFullPath2, FileMode.Open))
+            {
+               hashFile2 = hashAlgorithm.ComputeHash(fs);
+            }
          }
 
          return BitConverter.ToString(hashFile1) == BitConverter.ToString(hashFile2);

src/OSPSuite.Utility/OSPSuite.Utility.csproj

@@ -7,7 +7,7 @@
     <GenerateAssemblyProductAttribute>false</GenerateAssemblyProductAttribute>
     <GenerateAssemblyCompanyAttribute>false</GenerateAssemblyCompanyAttribute>
     <PackageProjectUrl>https://github.com/Open-Systems-Pharmacology/OSPSuite.Utility</PackageProjectUrl>
-    <PackageIconUrl>https://raw.githubusercontent.com/Open-Systems-Pharmacology/Suite/master/logo.png</PackageIconUrl>
+    <PackageIcon>logo.png</PackageIcon>
     <RepositoryUrl>https://github.com/Open-Systems-Pharmacology/OSPSuite.Utility</RepositoryUrl>
     <PackageTags>open-systems-pharmacology, ospsuite-components</PackageTags>
     <Description>Utility methods and classes for the Open Systems Pharmacology Suite</Description>
@@ -27,6 +27,10 @@
       <Pack>True</Pack>
       <PackagePath></PackagePath>
     </None>
+    <None Include="..\..\logo.png">
+      <Pack>True</Pack>
+      <PackagePath></PackagePath>
+    </None>
   </ItemGroup>
 
 

tests/OSPSuite.Utility.Tests/BuilderRepositorySpecs.cs

@@ -1,5 +1,7 @@
 using System;
 using System.Collections.Generic;
+using System.Linq;
+using System.Threading;
 using FakeItEasy;
 using OSPSuite.BDDHelper;
 using OSPSuite.BDDHelper.Extensions;
@@ -10,7 +12,7 @@ namespace OSPSuite.Utility.Tests
 {
    public abstract class concern_for_BuilderRepository : ContextSpecification<IBuilderRepository<IMyBuilder>>
    {
-      private IContainer _container;
+      protected IContainer _container;
       protected List<IMyBuilder> _allTEXBuilder;
 
       protected override void Context()
@@ -48,6 +50,63 @@ public void should_use_the_most_accurate_builder()
       }
    }
 
+   public class When_resolving_a_builder_concurrently_for_the_same_type : concern_for_BuilderRepository
+   {
+      private List<Exception> _exceptions;
+      private const int _threadCount = 16;
+      private const int _attempts = 100;
+
+      protected override void Context()
+      {
+         base.Context();
+         // exactly one builder matches MySuperParameter, so BuilderFor takes the single-builder Add path
+         _allTEXBuilder.Add(new TEXMySuperBuilder());
+         _exceptions = new List<Exception>();
+      }
+
+      protected override void Because()
+      {
+         // The cache→Add race only exists during the first population of a given type, so each
+         // attempt uses a fresh repository to re-open that window, with all threads released at once.
+         for (var attempt = 0; attempt < _attempts; attempt++)
+         {
+            var repository = new BuilderRepositoryForSpecs(_container);
+            var startSignal = new ManualResetEventSlim(false);
+            var threads = new List<Thread>();
+
+            for (var t = 0; t < _threadCount; t++)
+            {
+               threads.Add(new Thread(() =>
+               {
+                  startSignal.Wait();
+                  try
+                  {
+                     repository.BuilderFor(new MySuperParameter());
+                  }
+                  catch (Exception ex)
+                  {
+                     lock (_exceptions)
+                     {
+                        _exceptions.Add(ex);
+                     }
+                  }
+               }));
+            }
+
+            threads.ForEach(x => x.Start());
+            startSignal.Set();
+            threads.ForEach(x => x.Join());
+         }
+      }
+
+      [Observation]
+      public void should_resolve_the_builder_without_throwing()
+      {
+         var messages = string.Join(Environment.NewLine, _exceptions.Select(ex => $"{ex.GetType().Name}: {ex.Message}"));
+         messages.ShouldBeEqualTo(string.Empty);
+      }
+   }
+
    internal class Parameter
    {
    }