From a8c5763c20ec92a5a52c1925edad49df05326c9e Mon Sep 17 00:00:00 2001 From: Bernhard Richter Date: Tue, 19 May 2026 13:19:17 +0200 Subject: [PATCH 1/3] Fix thread safety of disposableObjects in ServiceContainer.Dispose TrackInstance and Dispose both accessed the disposableObjects list without synchronization, causing "collection was modified" errors under concurrent load. - Lock disposableObjects.Add in TrackInstance using the existing lockObject - Atomically snapshot-and-clear disposableObjects under lock in Dispose, then dispose the snapshot outside the lock to avoid holding the lock during user code - Use a reference-equality HashSet to skip duplicates, consistent with Scope.Dispose - Add DisposableObjectComparer nested class for stable identity-based hashing - Add concurrent stress test (1000 iterations, 3 threads) as regression guard Co-Authored-By: Claude Sonnet 4.6 --- src/LightInject.Tests/DisposableTests.cs | 61 ++++++++++++++++++++++++ src/LightInject/LightInject.cs | 31 ++++++++---- 2 files changed, 84 insertions(+), 8 deletions(-) diff --git a/src/LightInject.Tests/DisposableTests.cs b/src/LightInject.Tests/DisposableTests.cs index f1ee1082..e822ab29 100644 --- a/src/LightInject.Tests/DisposableTests.cs +++ b/src/LightInject.Tests/DisposableTests.cs @@ -1,7 +1,10 @@ using System; +using System.Collections.Concurrent; using System.Collections.Generic; using System.Diagnostics.CodeAnalysis; using System.Runtime.CompilerServices; +using System.Threading; +using System.Threading.Tasks; using LightInject.SampleLibrary; using Xunit; namespace LightInject.Tests @@ -180,6 +183,52 @@ public void Dispose_Scope_CallsCompletedHandler() //} + [Fact] + public void Dispose_ConcurrentWithServiceCreation_DoesNotThrow() + { + var exceptions = new ConcurrentBag(); + + for (int attempt = 0; attempt < 1000 && exceptions.IsEmpty; attempt++) + { + var container = new ServiceContainer(); + for (int i = 0; i < 100; i++) + { + int captured = i; + container.Register(_ => new DisposableFoo(), $"s{captured}", new PerContainerLifetime()); + } + + using var barrier = new Barrier(3); + + var task1 = Task.Run(() => + { + barrier.SignalAndWait(); + for (int i = 0; i < 50; i++) + { + try { container.GetInstance($"s{i}"); } + catch (Exception ex) { exceptions.Add(ex); break; } + } + }); + + var task2 = Task.Run(() => + { + barrier.SignalAndWait(); + for (int i = 50; i < 100; i++) + { + try { container.GetInstance($"s{i}"); } + catch (Exception ex) { exceptions.Add(ex); break; } + } + }); + + barrier.SignalAndWait(); + try { container.Dispose(); } + catch (Exception ex) { exceptions.Add(ex); } + + Task.WaitAll(task1, task2); + } + + Assert.Empty(exceptions); + } + private static IServiceContainer CreateContainer() { return new ServiceContainer(); @@ -317,6 +366,18 @@ public FakeDisposableCallbackOuterService(IFakeService singleService, IEnumerabl MultipleServices = multipleServices; } } + + public class ActionDisposable : IFoo, IDisposable + { + private readonly Action onDispose; + + public ActionDisposable(Action onDispose) + { + this.onDispose = onDispose; + } + + public void Dispose() => onDispose(); + } } /// diff --git a/src/LightInject/LightInject.cs b/src/LightInject/LightInject.cs index a9cecdf8..de0f1988 100644 --- a/src/LightInject/LightInject.cs +++ b/src/LightInject/LightInject.cs @@ -3480,17 +3480,20 @@ public void Dispose() disposableLifetimeInstance.Dispose(); } - List disposedObjects = new List(); - var perContainerDisposables = disposableObjects.AsEnumerable().Reverse(); - foreach (var perContainerDisposable in perContainerDisposables) + IDisposable[] snapshot; + lock (lockObject) { - disposedObjects.Add(perContainerDisposable); - perContainerDisposable.Dispose(); + snapshot = disposableObjects.ToArray(); + disposableObjects.Clear(); } - foreach (var disposed in disposedObjects) + var seen = new HashSet(DisposableObjectComparer.Default); + foreach (var disposable in snapshot.Reverse()) { - disposableObjects.Remove(disposed); + if (seen.Add(disposable)) + { + disposable.Dispose(); + } } } @@ -3527,7 +3530,10 @@ internal static TService TrackInstance(TService instance, ServiceConta { if (instance is IDisposable disposable) { - container.disposableObjects.Add(disposable); + lock (container.lockObject) + { + container.disposableObjects.Add(disposable); + } } return instance; @@ -5438,6 +5444,15 @@ public ServiceRegistration Execute(IServiceFactory serviceFactory, ServiceRegist } } } + + private class DisposableObjectComparer : IEqualityComparer + { + public static readonly DisposableObjectComparer Default = new DisposableObjectComparer(); + + public bool Equals(IDisposable x, IDisposable y) => ReferenceEquals(x, y); + + public int GetHashCode(IDisposable obj) => System.Runtime.CompilerServices.RuntimeHelpers.GetHashCode(obj); + } } /// From b9dae0abe23abb21ea08e970d9a45ce304810cc6 Mon Sep 17 00:00:00 2001 From: Bernhard Richter Date: Tue, 19 May 2026 13:23:15 +0200 Subject: [PATCH 2/3] Disable isolated load context --- .github/workflows/main.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 6153580e..ae3fe89d 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -19,7 +19,7 @@ jobs: run: dotnet tool install --global dotnet-ilverify --version 8.0.0 - name: Run build script - run: dotnet-script build/build.csx + run: dotnet-script build/build.csx --disable-isolated-load-context env: # Or as an environment variable GITHUB_REPO_TOKEN: ${{ secrets.GITHUB_TOKEN }} IS_SECURE_BUILDENVIRONMENT: ${{ secrets.IS_SECURE_BUILDENVIRONMENT }} From fdca368379f605fc8465c79be9dc722c4292df8f Mon Sep 17 00:00:00 2001 From: Bernhard Richter Date: Tue, 19 May 2026 13:39:27 +0200 Subject: [PATCH 3/3] Add tests to restore 100% line coverage Cover the duplicate-detection continue branches in Scope.DisposeAsync() (fast IAsyncDisposable path, IDisposable path, and slow async Await path) and the DisposableObjectComparer.Equals path in ServiceContainer.Dispose(). Co-Authored-By: Claude Sonnet 4.6 --- src/LightInject.Tests/AsyncDisposableTests.cs | 53 +++++++++++++++++++ src/LightInject.Tests/DisposableTests.cs | 18 +++++++ 2 files changed, 71 insertions(+) diff --git a/src/LightInject.Tests/AsyncDisposableTests.cs b/src/LightInject.Tests/AsyncDisposableTests.cs index df8aaf76..0214195e 100644 --- a/src/LightInject.Tests/AsyncDisposableTests.cs +++ b/src/LightInject.Tests/AsyncDisposableTests.cs @@ -115,6 +115,59 @@ public void ShouldThrowWhenAsyncDisposableIsDisposedInSynchronousScope() Assert.Throws(() => scope.Dispose()); } + [Fact] + public async Task DisposeAsync_DuplicateAsyncDisposable_IsDisposedOnce() + { + var container = CreateContainer(); + var disposeCount = 0; + var instance = new AsyncDisposable(_ => disposeCount++); + + await using (var scope = container.BeginScope()) + { + scope.TrackInstance(instance); + scope.TrackInstance(instance); + } + + Assert.Equal(1, disposeCount); + } + + [Fact] + public async Task DisposeAsync_DuplicateDisposable_IsDisposedOnce() + { + var container = CreateContainer(); + var disposeCount = 0; + var instance = new Disposable(_ => disposeCount++); + + await using (var scope = container.BeginScope()) + { + scope.TrackInstance(instance); + scope.TrackInstance(instance); + } + + Assert.Equal(1, disposeCount); + } + + [Fact] + public async Task DisposeAsync_DuplicateAsyncDisposableWithSlowDisposable_IsDisposedOnce() + { + var container = CreateContainer(); + var disposeCount = 0; + var instance = new AsyncDisposable(_ => disposeCount++); + + await using (var scope = container.BeginScope()) + { + // instance at index 0 and 1; SlowAsyncDisposable at index 2. + // DisposeAsync processes highest index first: SlowAsyncDisposable triggers + // the Await path, which then processes index 1 (disposes instance) and + // index 0 (duplicate — hits the continue branch in Await). + scope.TrackInstance(instance); + scope.TrackInstance(instance); + scope.TrackInstance(new SlowAsyncDisposable(_ => { })); + } + + Assert.Equal(1, disposeCount); + } + public class SlowAsyncDisposable : IAsyncDisposable { private readonly Action onDisposed; diff --git a/src/LightInject.Tests/DisposableTests.cs b/src/LightInject.Tests/DisposableTests.cs index e822ab29..49c02b66 100644 --- a/src/LightInject.Tests/DisposableTests.cs +++ b/src/LightInject.Tests/DisposableTests.cs @@ -183,6 +183,24 @@ public void Dispose_Scope_CallsCompletedHandler() //} + [Fact] + public void Dispose_SharedInstanceRegisteredUnderMultipleNames_IsDisposedOnce() + { + var container = CreateContainer(); + var disposeCount = 0; + var shared = new ActionDisposable(() => disposeCount++); + + container.Register(_ => shared, "first", new PerContainerLifetime()); + container.Register(_ => shared, "second", new PerContainerLifetime()); + + container.GetInstance("first"); + container.GetInstance("second"); + + container.Dispose(); + + Assert.Equal(1, disposeCount); + } + [Fact] public void Dispose_ConcurrentWithServiceCreation_DoesNotThrow() {