diff --git a/docs/docs/writing-tests/class-data-source.md b/docs/docs/writing-tests/class-data-source.md index 9fc3da12055..fc53b2381de 100644 --- a/docs/docs/writing-tests/class-data-source.md +++ b/docs/docs/writing-tests/class-data-source.md @@ -170,6 +170,35 @@ public class ProductTests Use `IAsyncInitializer`, not `IAsyncDiscoveryInitializer`, for this work. `IAsyncDiscoveryInitializer` runs while tests are built. See the [test lifecycle](lifecycle.md) for the full order. +## When Shared Instances Are Disposed + +TUnit counts the tests that hold a shared instance. When the last of them ends, TUnit disposes the instance. A test that asks for the same instance after that, such as a dynamic test added later in the run, gets a new instance. + +A test can also fetch a shared instance itself with `SharedDataSources.GetOrCreate`. It gets the same instance that `ClassDataSource` injects with the same `SharedType` (and key or class). While the test runs, it holds the instance, so TUnit does not dispose it before the test ends. This covers the test class constructor, the test method, and `[Before(Test)]` and `[After(Test)]` hooks: an instance fetched in any of them is held by that test, and is disposed after the test ends if no other test holds it: + + +```csharp +public class Database : IAsyncDisposable +{ + public ValueTask DisposeAsync() => ValueTask.CompletedTask; +} + +public class ReportTests +{ + [Test] + public async Task Reads_From_Shared_Database() + { + var database = SharedDataSources.GetOrCreate(SharedType.PerTestSession); + + await Assert.That(database).IsNotNull(); + } +} +``` + +`GetOrCreate` does not call `IAsyncInitializer.InitializeAsync`. Only instances that TUnit injects are initialized. + +Outside a running test, nothing holds a fetched instance. This includes `[Before(Class)]`, `[After(Class)]`, `[Before(Assembly)]`, `[After(Assembly)]`, `[Before(TestSession)]` and `[After(TestSession)]` hooks, and background code. TUnit disposes it when the last test that holds it ends, which can be while you still use it. If no test holds it, TUnit does not dispose it. To keep a shared instance for the whole session, inject it into a static property with `ClassDataSource` instead. + # Class Data Source Overloads If you are using an overload that supports injecting multiple classes at once (e.g. `ClassDataSource`) then you should specify multiple SharedTypes in an array and keys where applicable. diff --git a/src/TUnit.Core/SharedDataSources.cs b/src/TUnit.Core/SharedDataSources.cs index a6e6fa4aae1..f5b116ae262 100644 --- a/src/TUnit.Core/SharedDataSources.cs +++ b/src/TUnit.Core/SharedDataSources.cs @@ -1,6 +1,7 @@ using System.Diagnostics.CodeAnalysis; using TUnit.Core.Helpers; using TUnit.Core.Interfaces; +using TUnit.Core.Tracking; namespace TUnit.Core; @@ -64,15 +65,12 @@ public static T GetOrCreate(SharedType sharedType, Type? testClassType, strin { _ = factory ?? throw new ArgumentNullException(nameof(factory)); - return sharedType switch + if (sharedType == SharedType.None) { - SharedType.None => factory(), - SharedType.PerTestSession => (T)TestDataContainer.GetGlobalInstance(typeof(T), _ => factory()!)!, - SharedType.PerClass => GetForClass(testClassType, factory), - SharedType.Keyed => GetForKey(key, factory), - SharedType.PerAssembly => GetForAssembly(testClassType, factory), - _ => throw new ArgumentOutOfRangeException(nameof(sharedType), sharedType, "Invalid SharedType value.") - }; + return factory(); + } + + return (T)GetShared(sharedType, typeof(T), testClassType, key, () => factory())!; } /// @@ -144,9 +142,42 @@ public static T GetOrCreate(SharedType sharedType, DataGeneratorMetadata data _ = type ?? throw new ArgumentNullException(nameof(type)); _ = factory ?? throw new ArgumentNullException(nameof(factory)); + if (sharedType == SharedType.None) + { + return factory(); + } + + return GetShared(sharedType, type, testClassType, key, factory); + } + + /// + /// Fetches a shared instance. Inside a running test, the test holds a reference to it until the + /// test ends, so it is not disposed while the test uses it, and is disposed afterwards if no + /// other test holds it. If the instance was disposed before the test could hold it, a new one + /// is created. + /// + private static object? GetShared(SharedType sharedType, Type type, Type? testClassType, string? key, Func factory) + { + var instance = Resolve(sharedType, type, testClassType, key, factory); + + if (instance is null || TestContext.Current is not { } testContext) + { + return instance; + } + + // Fails only when the instance has just been retired, which also evicts it from the cache. + while (!ObjectTracker.TryHoldForRunningTest(testContext, instance)) + { + instance = Resolve(sharedType, type, testClassType, key, factory)!; + } + + return instance; + } + + private static object? Resolve(SharedType sharedType, Type type, Type? testClassType, string? key, Func factory) + { return sharedType switch { - SharedType.None => factory(), SharedType.PerTestSession => TestDataContainer.GetGlobalInstance(type, _ => factory()!), SharedType.PerClass => GetForClass(type, testClassType, factory), SharedType.Keyed => GetForKey(type, key, factory), @@ -155,16 +186,6 @@ public static T GetOrCreate(SharedType sharedType, DataGeneratorMetadata data }; } - private static T GetForClass(Type? testClassType, Func factory) - { - if (testClassType is null) - { - throw new ArgumentNullException(nameof(testClassType), "testClassType is required when SharedType is PerClass."); - } - - return (T)TestDataContainer.GetInstanceForClass(testClassType, typeof(T), _ => factory()!)!; - } - private static object? GetForClass(Type type, Type? testClassType, Func factory) { if (testClassType is null) @@ -175,26 +196,6 @@ private static T GetForClass(Type? testClassType, Func factory) return TestDataContainer.GetInstanceForClass(testClassType, type, _ => factory()!); } - private static T GetForKey(string? key, Func factory) - { - if (string.IsNullOrEmpty(key)) - { - throw new ArgumentNullException(nameof(key), "key is required when SharedType is Keyed."); - } - - return (T)TestDataContainer.GetInstanceForKey(key!, typeof(T), _ => - { - var instance = factory()!; - - if (instance is IKeyedDataSource keyed) - { - keyed.Key = key!; - } - - return instance; - })!; - } - private static object? GetForKey(Type type, string? key, Func factory) { if (string.IsNullOrEmpty(key)) @@ -215,16 +216,6 @@ private static T GetForKey(string? key, Func factory) }); } - private static T GetForAssembly(Type? testClassType, Func factory) - { - if (testClassType is null) - { - throw new ArgumentNullException(nameof(testClassType), "testClassType is required when SharedType is PerAssembly."); - } - - return (T)TestDataContainer.GetInstanceForAssembly(testClassType.Assembly, typeof(T), _ => factory()!)!; - } - private static object? GetForAssembly(Type type, Type? testClassType, Func factory) { if (testClassType is null) diff --git a/src/TUnit.Core/TestContext.cs b/src/TUnit.Core/TestContext.cs index 259b507e395..2f62b566c45 100644 --- a/src/TUnit.Core/TestContext.cs +++ b/src/TUnit.Core/TestContext.cs @@ -495,6 +495,62 @@ internal SortedList> TrackedObjects internal SortedList>? TrackedObjectsIfCreated => Volatile.Read(ref _trackedObjects); + private int _holdsSharedReferences; + + /// + /// True while the test's own lifecycle runs (from instance creation until its tracked objects + /// are untracked). Shared instances that the test fetches through + /// during this window are held by the test (). + /// False during discovery and registration, where data-source objects are counted by the + /// object-graph walk instead. + /// + internal bool HoldsSharedReferences => Volatile.Read(ref _holdsSharedReferences) == 1; + + internal void StartHoldingSharedReferences() => Volatile.Write(ref _holdsSharedReferences, 1); + + private HashSet? _heldSharedInstances; + + /// + /// Shared instances the test fetched itself through while + /// was true. Each holds one reference, released when the + /// test's tracked objects are untracked. Kept apart from so the + /// engine does not initialize them and its initialization walk never sees the set change. + /// Lock on the set itself to read or change it. + /// + internal HashSet HeldSharedInstances + { + get + { + if (Volatile.Read(ref _heldSharedInstances) is { } existing) + { + return existing; + } + + var created = new HashSet(Helpers.ReferenceEqualityComparer.Instance); + return Interlocked.CompareExchange(ref _heldSharedInstances, created, null) ?? created; + } + } + + internal HashSet? HeldSharedInstancesIfCreated => Volatile.Read(ref _heldSharedInstances); + + /// + /// Stops new shared references being added, and waits for any add in progress, so the held + /// instances can be untracked safely. + /// + internal void StopHoldingSharedReferences() + { + // Full fence: either an adder that creates the held set after this sees the flag + // cleared, or this sees the set and waits on its lock. + Interlocked.Exchange(ref _holdsSharedReferences, 0); + + if (HeldSharedInstancesIfCreated is { } heldSharedInstances) + { + lock (heldSharedInstances) + { + } + } + } + /// /// Tracked objects that are reachable only from data rows the test never receives /// (). They are tracked for reference-counted diff --git a/src/TUnit.Core/TestDataContainer.cs b/src/TUnit.Core/TestDataContainer.cs index 38fbc72e9f7..1dca46d3bd5 100644 --- a/src/TUnit.Core/TestDataContainer.cs +++ b/src/TUnit.Core/TestDataContainer.cs @@ -1,33 +1,60 @@ -using System.Reflection; -using TUnit.Core.Data; +using System.Collections.Concurrent; +using System.Reflection; namespace TUnit.Core; internal static class TestDataContainer { - private static readonly ScopedDictionary _globalContainer = new(); - private static readonly ScopedDictionary _classContainer = new(); - private static readonly ScopedDictionary _assemblyContainer = new(); - private static readonly ScopedDictionary _keyContainer = new(); + private static readonly object GlobalScope = new(); + + private static readonly ConcurrentDictionary> _instances = new(); + + // Disposable cached instances only, by identity, so that a disposed instance can be evicted. + // Without eviction, a test registered after the last holder has disposed a shared instance + // (e.g. a dynamic test) would be handed the disposed instance (#7204). A factory can return + // the same instance for several keys or types, so every entry holding it is recorded. + private static readonly ConcurrentDictionary>[]> _entriesByInstance = + new(Helpers.ReferenceEqualityComparer.Instance); public static object? GetInstanceForClass(Type testClass, Type type, Func func) { - return _classContainer.GetOrCreate(testClass, type, func); + return GetOrCreate(new InstanceKey(SharedType.PerClass, testClass, type), func); } public static object? GetInstanceForAssembly(Assembly assembly, Type type, Func func) { - return _assemblyContainer.GetOrCreate(assembly, type, func); + return GetOrCreate(new InstanceKey(SharedType.PerAssembly, assembly, type), func); } public static object? GetGlobalInstance(Type type, Func func) { - return _globalContainer.GetOrCreate(typeof(object).FullName!, type, func); + return GetOrCreate(new InstanceKey(SharedType.PerTestSession, GlobalScope, type), func); } public static object? GetInstanceForKey(string key, Type type, Func func) { - return _keyContainer.GetOrCreate(key, type, func); + return GetOrCreate(new InstanceKey(SharedType.Keyed, key, type), func); + } + + /// + /// Whether is a disposable instance that is still cached. + /// + public static bool IsCached(object instance) => _entriesByInstance.ContainsKey(instance); + + /// + /// Removes from the cache, so the next request for any scope and + /// type it is cached under creates a new instance. Does nothing if it is not cached. + /// + public static void Evict(object instance) + { + if (_entriesByInstance.TryRemove(instance, out var entries)) + { + foreach (var entry in entries) + { + // Removes the entry only while it still holds this instance's Lazy (reference equality). + ((ICollection>>) _instances).Remove(entry); + } + } } /// @@ -37,9 +64,58 @@ internal static class TestDataContainer /// public static void Reset() { - _globalContainer.Clear(); - _classContainer.Clear(); - _assemblyContainer.Clear(); - _keyContainer.Clear(); + _instances.Clear(); + _entriesByInstance.Clear(); + } + + private static object? GetOrCreate(InstanceKey key, Func func) + { + if (_instances.TryGetValue(key, out var existing)) + { + return existing.Value; + } + + return AddSlow(key, func); + } + + // Kept separate from GetOrCreate so the fast path does not allocate the closure. + private static object? AddSlow(InstanceKey key, Func func) + { + Lazy lazy = null!; + lazy = new Lazy(() => + { + var instance = func(key.Type); + + if (instance is IDisposable or IAsyncDisposable) + { + var entry = new KeyValuePair>(key, lazy); + _entriesByInstance.AddOrUpdate(instance, _ => [entry], (_, entries) => [..entries, entry]); + } + + return instance; + }, LazyThreadSafetyMode.ExecutionAndPublication); + + return _instances.GetOrAdd(key, lazy).Value; + } + + private readonly struct InstanceKey(SharedType kind, object scope, Type type) : IEquatable + { + public SharedType Kind { get; } = kind; + + public object Scope { get; } = scope; + + public Type Type { get; } = type; + + public bool Equals(InstanceKey other) => Kind == other.Kind && Type == other.Type && Scope.Equals(other.Scope); + + public override bool Equals(object? obj) => obj is InstanceKey other && Equals(other); + + public override int GetHashCode() + { + unchecked + { + return ((int) Kind * 397 ^ Scope.GetHashCode()) * 397 ^ Type.GetHashCode(); + } + } } } diff --git a/src/TUnit.Core/Tracking/ObjectTracker.cs b/src/TUnit.Core/Tracking/ObjectTracker.cs index bdd252645d4..b81475e2e59 100644 --- a/src/TUnit.Core/Tracking/ObjectTracker.cs +++ b/src/TUnit.Core/Tracking/ObjectTracker.cs @@ -19,6 +19,10 @@ internal class ObjectTracker(TrackableObjectGraphProvider trackableObjectGraphPr private static readonly ConcurrentDictionary s_trackedObjects = new(Helpers.ReferenceEqualityComparer.Instance); + // Serializes retiring a cached shared instance (count reached zero) with a running test taking + // a reference to it, so the instance is either retired or held, never both. + private static readonly object s_sharedInstanceLock = new(); + // Collects errors from async disposal callbacks for post-session review private static readonly ConcurrentBag s_asyncCallbackErrors = new(); @@ -185,57 +189,74 @@ private static void RemoveObjectsNotIn(SortedList>? tracked public ValueTask?> UntrackObjects(TestContext testContext) { var trackedObjects = testContext.TrackedObjectsIfCreated; + var heldSharedInstances = testContext.HeldSharedInstancesIfCreated; - if (CountTrackedObjects(trackedObjects) == 0) + if (CountTrackedObjects(trackedObjects) == 0 && heldSharedInstances is not { Count: > 0 }) { return new ValueTask?>((List?)null); } - return UntrackObjectsAsync(trackedObjects!); + return UntrackObjectsAsync(trackedObjects, heldSharedInstances); } - private async ValueTask?> UntrackObjectsAsync(SortedList> trackedObjects) + private async ValueTask?> UntrackObjectsAsync(SortedList>? trackedObjects, HashSet? heldSharedInstances) { // SortedList keeps keys in ascending order; iterate in ascending order (shallowest depth first). // This ensures disposal happens in reverse order of initialization (which goes deepest first). // Dependents (shallow) are disposed before their dependencies (deep). List? cleanupExceptions = null; - var values = trackedObjects.Values; - for (var i = 0; i < values.Count; i++) + if (trackedObjects is not null) { - var bucket = values[i]; - List? disposalTasks = null; + var values = trackedObjects.Values; - foreach (var obj in bucket) + for (var i = 0; i < values.Count; i++) { - try - { - var task = UntrackObject(obj); + cleanupExceptions = await UntrackBucketAsync(values[i], cleanupExceptions).ConfigureAwait(false); + } + } - if (!task.IsCompletedSuccessfully) - { - disposalTasks ??= []; - disposalTasks.Add(task.AsTask()); - } - } - catch (Exception e) + // Released last: the test's own objects may depend on the shared instances it fetched. + if (heldSharedInstances is { Count: > 0 }) + { + cleanupExceptions = await UntrackBucketAsync(heldSharedInstances, cleanupExceptions).ConfigureAwait(false); + } + + return cleanupExceptions; + } + + private async ValueTask?> UntrackBucketAsync(HashSet bucket, List? cleanupExceptions) + { + List? disposalTasks = null; + + foreach (var obj in bucket) + { + try + { + var task = UntrackObject(obj); + + if (!task.IsCompletedSuccessfully) { - (cleanupExceptions ??= []).Add(e); + disposalTasks ??= []; + disposalTasks.Add(task.AsTask()); } } + catch (Exception e) + { + (cleanupExceptions ??= []).Add(e); + } + } - if (disposalTasks is { Count: > 0 }) + if (disposalTasks is { Count: > 0 }) + { + var whenAllTask = Task.WhenAll(disposalTasks); + try { - var whenAllTask = Task.WhenAll(disposalTasks); - try - { - await whenAllTask.ConfigureAwait(false); - } - catch - { - (cleanupExceptions ??= []).AddRange(whenAllTask.Exception!.InnerExceptions); - } + await whenAllTask.ConfigureAwait(false); + } + catch + { + (cleanupExceptions ??= []).AddRange(whenAllTask.Exception!.InnerExceptions); } } @@ -303,10 +324,9 @@ private async ValueTask UntrackObject(object? obj) if (count == 0) { - // Remove from tracking dictionary to prevent memory leak - // Use TryRemove to ensure atomicity - only remove if still in dictionary - s_trackedObjects.TryRemove(obj, out _); - shouldDispose = true; + shouldDispose = TestDataContainer.IsCached(obj) + ? TryRetireSharedInstance(obj, counter) + : RemoveCounter(obj, counter); } } @@ -319,6 +339,105 @@ private async ValueTask UntrackObject(object? obj) } } + /// + /// Removes from the tracking dictionary (preventing a memory leak), + /// but only while it is still the object's counter. Returns whether it was removed. The caller + /// disposes the object only then, so two callers that both saw the count reach zero (a shared + /// instance held again by a running test in between) cannot both dispose it. + /// + private static bool RemoveCounter(object obj, Counter counter) => + ((ICollection>) s_trackedObjects).Remove(new KeyValuePair(obj, counter)); + + /// + /// Stops tracking a cached shared instance whose count has reached zero and evicts it from + /// , so later requests create a new instance instead of + /// receiving this one after it is disposed (#7204). Returns false when a running test took a + /// reference () after the count reached zero, or when that + /// test has already retired it. + /// + private static bool TryRetireSharedInstance(object obj, Counter counter) + { + lock (s_sharedInstanceLock) + { + // Fails if a running test took a reference after the count reached zero, or if that + // test has since released it and retired the instance itself. + if (counter.CurrentCount != 0 || !RemoveCounter(obj, counter)) + { + return false; + } + + TestDataContainer.Evict(obj); + return true; + } + } + + /// + /// Makes a running test hold a reference to a shared instance that it fetched itself (through + /// ) rather than receiving it from a data source. The reference + /// is released, and the instance disposed if no other test holds it, when the test's tracked + /// objects are untracked at the end of the test. + /// + /// + /// False only when the instance has already been retired (disposed or about to be), in which + /// case the caller must fetch the instance again; the cache then creates a new one. + /// True otherwise, including when no test is running or the test already holds the instance. + /// + internal static bool TryHoldForRunningTest(TestContext testContext, object instance) + { + if (ShouldSkipTracking(instance) || !testContext.HoldsSharedReferences) + { + return true; + } + + var heldSharedInstances = testContext.HeldSharedInstances; + + // Code inside a test can fetch shared instances concurrently; the lock also lets + // TestContext.StopHoldingSharedReferences wait for any add in progress. + lock (heldSharedInstances) + { + // The tracked objects no longer change once the test runs, so they can be read here. + if (!testContext.HoldsSharedReferences + || heldSharedInstances.Contains(instance) + || Contains(testContext.TrackedObjectsIfCreated, instance)) + { + return true; + } + + lock (s_sharedInstanceLock) + { + if (!TestDataContainer.IsCached(instance)) + { + return false; + } + + GetOrCreateCounter(instance).Increment(); + } + + // Kept out of the tracked objects, so the engine does not initialize it. + heldSharedInstances.Add(instance); + return true; + } + } + + private static bool Contains(SortedList>? trackedObjects, object obj) + { + if (trackedObjects is null) + { + return false; + } + + var values = trackedObjects.Values; + for (var i = 0; i < values.Count; i++) + { + if (values[i].Contains(obj)) + { + return true; + } + } + + return false; + } + /// /// Determines if an object should be skipped from tracking. /// diff --git a/src/TUnit.Engine/Services/TestExecution/TestCoordinator.cs b/src/TUnit.Engine/Services/TestExecution/TestCoordinator.cs index 991fc98cea0..2ae212fa6d1 100644 --- a/src/TUnit.Engine/Services/TestExecution/TestCoordinator.cs +++ b/src/TUnit.Engine/Services/TestExecution/TestCoordinator.cs @@ -137,6 +137,9 @@ public async ValueTask ExecuteTestAsync(AbstractExecutableTest test, Cancellatio // constructor (#6192). Hook tasks are cached, so ExecuteAsync's later awaits are no-ops. await _testExecutor.EnsureClassAndAssemblyHooksExecutedAsync(test, cancellationToken).ConfigureAwait(false); + // From here until UntrackObjects, shared instances the test fetches itself are held by it. + test.Context.StartHoldingSharedReferences(); + // Flow AsyncLocals captured by Before(Assembly)/Before(Class) hooks into the // constructor. Must run here (not inside the gate above) so the restored ambient // ExecutionContext is still in effect when CreateInstanceAsync runs the constructor. @@ -221,6 +224,7 @@ await RetryHelper.ExecuteWithRetry(test.Context, // Stay null on the success path — materializing the list only when something actually fails // saves ~56 bytes per passing test (and passing is the overwhelming majority). + test.Context.StopHoldingSharedReferences(); var cleanupExceptions = await _objectTracker.UntrackObjects(test.Context).ConfigureAwait(false); #if NET @@ -386,6 +390,9 @@ private async ValueTask ExecuteTestLifecycleAsync(AbstractExecutableTest test, C // instance so hooks precede the constructor (#6192). Cached, so ExecuteAsync re-awaits are no-ops. await _testExecutor.EnsureClassAndAssemblyHooksExecutedAsync(test, cancellationToken).ConfigureAwait(false); + // From here until UntrackObjects, shared instances the test fetches itself are held by it. + test.Context.StartHoldingSharedReferences(); + // Flow AsyncLocals captured by Before(Assembly)/Before(Class) hooks into the constructor. // Must run here (not inside the gate above) so the restored ambient ExecutionContext is still // in effect when CreateInstanceAsync runs the constructor. diff --git a/tests/TUnit.Engine.Tests/Issue7204Tests.cs b/tests/TUnit.Engine.Tests/Issue7204Tests.cs new file mode 100644 index 00000000000..b8172d2590f --- /dev/null +++ b/tests/TUnit.Engine.Tests/Issue7204Tests.cs @@ -0,0 +1,25 @@ +using Shouldly; +using TUnit.Engine.Tests.Enums; + +namespace TUnit.Engine.Tests; + +/// +/// A test that asks for a shared instance after its last holder has disposed it gets a new +/// instance: both a dynamic test registered later and a running test that fetches it through +/// SharedDataSources (#7204). +/// +public class Issue7204Tests(TestMode testMode) : InvokableTestBase(testMode) +{ + [Test] + public async Task Tests_Asking_After_Disposal_Get_A_New_Shared_Instance() + { + await RunTestsWithFilter( + "/*/TUnit.TestProject.Bugs._7204/*/*", + [ + result => result.ResultSummary.Outcome.ShouldBe("Completed"), + result => result.ResultSummary.Counters.Total.ShouldBe(6), + result => result.ResultSummary.Counters.Passed.ShouldBe(6), + result => result.ResultSummary.Counters.Failed.ShouldBe(0) + ]); + } +} diff --git a/tests/TUnit.TestProject/Bugs/_7204/ConstructorFetchedSharedInstanceTests.cs b/tests/TUnit.TestProject/Bugs/_7204/ConstructorFetchedSharedInstanceTests.cs new file mode 100644 index 00000000000..7db736b3c03 --- /dev/null +++ b/tests/TUnit.TestProject/Bugs/_7204/ConstructorFetchedSharedInstanceTests.cs @@ -0,0 +1,38 @@ +using TUnit.Core.Interfaces; +using TUnit.TestProject.Attributes; + +namespace TUnit.TestProject.Bugs._7204; + +// A shared instance that a test fetches itself through SharedDataSources is held by that test, but +// is not injected, so the engine must not initialize it. Its initializer fetching another shared +// instance must not break the test's setup either (#7204). + +public sealed class ConstructorFetchedInitializer : IAsyncInitializer, IDisposable +{ + public bool IsInitialized { get; private set; } + + public Task InitializeAsync() + { + // Synchronous, before any await: the fetch must not change a collection the engine is reading. + SharedDataSources.GetOrCreate(SharedType.Keyed, key: "7204-constructor-inner"); + IsInitialized = true; + return Task.CompletedTask; + } + + public void Dispose() + { + } +} + +[EngineTest(ExpectedResult.Pass)] +public class ConstructorFetchedSharedInstanceTests +{ + private readonly ConstructorFetchedInitializer _initializer = + SharedDataSources.GetOrCreate(SharedType.Keyed, key: "7204-constructor-outer"); + + [Test] + public async Task Instance_Fetched_In_Constructor_Is_Not_Initialized_By_The_Engine() + { + await Assert.That(_initializer.IsInitialized).IsFalse(); + } +} diff --git a/tests/TUnit.TestProject/Bugs/_7204/SharedInstanceAfterDisposalTests.cs b/tests/TUnit.TestProject/Bugs/_7204/SharedInstanceAfterDisposalTests.cs new file mode 100644 index 00000000000..2c51c7404d2 --- /dev/null +++ b/tests/TUnit.TestProject/Bugs/_7204/SharedInstanceAfterDisposalTests.cs @@ -0,0 +1,92 @@ +using TUnit.Core.Extensions; +using TUnit.TestProject.Attributes; + +#pragma warning disable WIP + +namespace TUnit.TestProject.Bugs._7204; + +// When the last test holding a shared instance ends, the instance is disposed. A test that asks +// for the same shared instance afterwards must get a new one, not the disposed one: both a +// dynamic test registered later and a running test that fetches it through SharedDataSources (#7204). + +public sealed class SharedAfterDisposalFixture : IDisposable +{ + public bool IsDisposed { get; private set; } + + public void Dispose() => IsDisposed = true; +} + +// Not a test class: only used as the class of the dynamic test, so its property is the only holder. +public sealed class SharedAfterDisposalDynamicConsumer +{ + public static SharedAfterDisposalFixture? Disposed { get; set; } + + [ClassDataSource(Shared = SharedType.Keyed, Key = SharedInstanceAfterDisposalTests.DynamicKey)] + public required SharedAfterDisposalFixture Fixture { get; init; } + + // Synchronous: the dynamic test's Action expression would discard a returned Task, and with it any failure. + public void Receives_A_Live_Instance() + { + if (Fixture.IsDisposed || ReferenceEquals(Fixture, Disposed)) + { + throw new InvalidOperationException("The dynamic test received the disposed shared instance."); + } + } +} + +[EngineTest(ExpectedResult.Pass)] +public class SharedInstanceAfterDisposalTests +{ + public const string DynamicKey = "7204-dynamic"; + private const string FetchedKey = "7204-fetched"; + + private static SharedAfterDisposalFixture? _dynamicHolderFixture; + private static SharedAfterDisposalFixture? _fetchedHolderFixture; + + [Test] + [ClassDataSource(Shared = SharedType.Keyed, Key = DynamicKey)] + public void Dynamic_Holder(SharedAfterDisposalFixture fixture) => _dynamicHolderFixture = fixture; + + [Test] + [ClassDataSource(Shared = SharedType.Keyed, Key = FetchedKey)] + public void Fetched_Holder(SharedAfterDisposalFixture fixture) => _fetchedHolderFixture = fixture; + + [Test, DependsOn(nameof(Dynamic_Holder), new[] { typeof(SharedAfterDisposalFixture) })] + public async Task Dynamic_Test_Registered_After_Disposal_Gets_A_New_Instance() + { + await WaitUntilDisposed(_dynamicHolderFixture!); + SharedAfterDisposalDynamicConsumer.Disposed = _dynamicHolderFixture; + + await TestContext.Current!.AddDynamicTest(new DynamicTest + { + TestMethod = consumer => consumer.Receives_A_Live_Instance(), + Attributes = [] + }); + } + + [Test, DependsOn(nameof(Fetched_Holder), new[] { typeof(SharedAfterDisposalFixture) })] + public async Task Running_Test_Fetching_After_Disposal_Gets_A_New_Instance() + { + await WaitUntilDisposed(_fetchedHolderFixture!); + + var fixture = SharedDataSources.GetOrCreate(SharedType.Keyed, key: FetchedKey); + + await Assert.That(fixture.IsDisposed).IsFalse(); + await Assert.That(fixture).IsNotSameReferenceAs(_fetchedHolderFixture); + } + + // The holder's references are released just after it is reported complete, so wait for that. + private static async Task WaitUntilDisposed(SharedAfterDisposalFixture fixture) + { + var deadline = DateTime.UtcNow.AddSeconds(30); + while (!fixture.IsDisposed) + { + if (DateTime.UtcNow > deadline) + { + throw new TimeoutException("The holder's fixture was not disposed."); + } + + await Task.Delay(10); + } + } +} diff --git a/tests/TUnit.UnitTests/SharedInstanceLifetimeTests.cs b/tests/TUnit.UnitTests/SharedInstanceLifetimeTests.cs new file mode 100644 index 00000000000..d22365f55e9 --- /dev/null +++ b/tests/TUnit.UnitTests/SharedInstanceLifetimeTests.cs @@ -0,0 +1,324 @@ +using TUnit.Core; +using TUnit.Core.Discovery; +using TUnit.Core.Interfaces; +using TUnit.Core.Tracking; + +namespace TUnit.UnitTests; + +/// +/// A shared instance whose last holder has ended is disposed and evicted, so a later request +/// creates a new instance instead of receiving the disposed one. A running test that fetches a +/// shared instance through holds it until the test ends (#7204). +/// +public class SharedInstanceLifetimeTests +{ + [Test] + public async Task Disposed_Shared_Instance_Is_Not_Handed_Out_Again() + { + var key = NewKey(); + var (tracker, discoverer) = CreateTracker(); + var holder = CreateContext(); + + var first = Fetch(key); + discoverer.Discover = context => Add(context, first); + tracker.TrackObjects(holder); + + await tracker.UntrackObjects(holder); + await Assert.That(first.IsDisposed).IsTrue(); + + var second = Fetch(key); + await Assert.That(second).IsNotSameReferenceAs(first); + await Assert.That(second.IsDisposed).IsFalse(); + } + + [Test] + public async Task Running_Test_Holds_Instance_It_Fetches_Until_It_Ends() + { + var key = NewKey(); + var (tracker, discoverer) = CreateTracker(); + var holder = CreateContext(); + var running = CreateContext(); + + var instance = Fetch(key); + discoverer.Discover = context => Add(context, instance); + tracker.TrackObjects(holder); + + running.StartHoldingSharedReferences(); + using (running.MakeCurrent()) + { + await Assert.That(SharedDataSources.GetOrCreate(SharedType.Keyed, key: key)).IsSameReferenceAs(instance); + } + + await tracker.UntrackObjects(holder); + await Assert.That(instance.IsDisposed).IsFalse(); + + running.StopHoldingSharedReferences(); + await tracker.UntrackObjects(running); + await Assert.That(instance.IsDisposed).IsTrue(); + } + + [Test] + public async Task Running_Test_Gets_A_New_Instance_After_The_Last_Holder_Disposed_It() + { + var key = NewKey(); + var (tracker, discoverer) = CreateTracker(); + var holder = CreateContext(); + var running = CreateContext(); + + var disposed = Fetch(key); + discoverer.Discover = context => Add(context, disposed); + tracker.TrackObjects(holder); + await tracker.UntrackObjects(holder); + + running.StartHoldingSharedReferences(); + SharedDisposable fetched; + using (running.MakeCurrent()) + { + fetched = SharedDataSources.GetOrCreate(SharedType.Keyed, key: key); + } + + await Assert.That(fetched).IsNotSameReferenceAs(disposed); + await Assert.That(fetched.IsDisposed).IsFalse(); + + running.StopHoldingSharedReferences(); + await tracker.UntrackObjects(running); + await Assert.That(fetched.IsDisposed).IsTrue(); + } + + [Test] + public async Task Fetching_An_Instance_The_Test_Already_Holds_Does_Not_Count_It_Twice() + { + var key = NewKey(); + var (tracker, discoverer) = CreateTracker(); + var running = CreateContext(); + + var instance = Fetch(key); + discoverer.Discover = context => Add(context, instance); + tracker.TrackObjects(running); + + running.StartHoldingSharedReferences(); + using (running.MakeCurrent()) + { + SharedDataSources.GetOrCreate(SharedType.Keyed, key: key); + SharedDataSources.GetOrCreate(SharedType.Keyed, key: key); + } + + running.StopHoldingSharedReferences(); + await tracker.UntrackObjects(running); + await Assert.That(instance.IsDisposed).IsTrue(); + } + + [Test] + public async Task Fetching_Outside_The_Running_Window_Does_Not_Hold() + { + var key = NewKey(); + var registering = CreateContext(); + var ended = CreateContext(); + ended.StartHoldingSharedReferences(); + ended.StopHoldingSharedReferences(); + + using (registering.MakeCurrent()) + { + SharedDataSources.GetOrCreate(SharedType.Keyed, key: key); + } + + using (ended.MakeCurrent()) + { + SharedDataSources.GetOrCreate(SharedType.Keyed, key: key); + } + + await Assert.That(registering.TrackedObjectsIfCreated?.Count ?? 0).IsEqualTo(0); + await Assert.That(registering.HeldSharedInstancesIfCreated?.Count ?? 0).IsEqualTo(0); + await Assert.That(ended.TrackedObjectsIfCreated?.Count ?? 0).IsEqualTo(0); + await Assert.That(ended.HeldSharedInstancesIfCreated?.Count ?? 0).IsEqualTo(0); + } + + [Test] + public async Task Held_Instances_Are_Kept_Out_Of_The_Tracked_Objects() + { + // The engine initializes everything in TrackedObjects, and an initializer can fetch another + // shared instance while that walk runs. Held instances must not be initialized or change + // the set the walk reads. + var key = NewKey(); + var (tracker, _) = CreateTracker(); + var running = CreateContext(); + + running.StartHoldingSharedReferences(); + SharedDisposable instance; + using (running.MakeCurrent()) + { + instance = SharedDataSources.GetOrCreate(SharedType.Keyed, key: key); + } + + await Assert.That(running.TrackedObjectsIfCreated?.Count ?? 0).IsEqualTo(0); + await Assert.That(running.HeldSharedInstances).Contains(instance); + + running.StopHoldingSharedReferences(); + await tracker.UntrackObjects(running); + await Assert.That(instance.IsDisposed).IsTrue(); + } + + [Test] + public async Task Instance_Cached_Under_Several_Keys_Is_Evicted_From_All_Of_Them() + { + var firstKey = NewKey(); + var secondKey = NewKey(); + var (tracker, discoverer) = CreateTracker(); + var holder = CreateContext(); + var running = CreateContext(); + + var shared = new SharedDisposable(); + using (CreateContext().MakeCurrent()) + { + SharedDataSources.GetOrCreate(SharedType.Keyed, (Type?) null, firstKey, () => shared); + SharedDataSources.GetOrCreate(SharedType.Keyed, (Type?) null, secondKey, () => shared); + } + + discoverer.Discover = context => Add(context, shared); + tracker.TrackObjects(holder); + await tracker.UntrackObjects(holder); + await Assert.That(shared.IsDisposed).IsTrue(); + + running.StartHoldingSharedReferences(); + var fetched = await Task.Run(() => + { + using (running.MakeCurrent()) + { + return ( + First: SharedDataSources.GetOrCreate(SharedType.Keyed, (Type?) null, firstKey, () => new SharedDisposable()), + Second: SharedDataSources.GetOrCreate(SharedType.Keyed, (Type?) null, secondKey, () => new SharedDisposable())); + } + }).WaitAsync(TimeSpan.FromSeconds(30)); + + await Assert.That(fetched.First).IsNotSameReferenceAs(shared); + await Assert.That(fetched.First.IsDisposed).IsFalse(); + await Assert.That(fetched.Second).IsNotSameReferenceAs(shared); + await Assert.That(fetched.Second.IsDisposed).IsFalse(); + + running.StopHoldingSharedReferences(); + await tracker.UntrackObjects(running); + } + + [Test] + public async Task Instance_Held_Again_After_Its_Count_Reached_Zero_Is_Disposed_Once() + { + // The last holder's count reaches zero; before it retires the instance, a running test + // holds the instance, ends, and retires and disposes it. The first holder must not then + // dispose it a second time. + var key = NewKey(); + var (tracker, discoverer) = CreateTracker(); + var holder = CreateContext(); + var running = CreateContext(); + + var instance = Fetch(key); + discoverer.Discover = context => Add(context, instance); + tracker.TrackObjects(holder); + + // Raised synchronously when the count reaches zero, before the holder retires the instance. + ObjectTracker.OnDisposed(instance, () => + { + running.StartHoldingSharedReferences(); + using (running.MakeCurrent()) + { + SharedDataSources.GetOrCreate(SharedType.Keyed, key: key); + } + + running.StopHoldingSharedReferences(); + var untracking = tracker.UntrackObjects(running); + if (!untracking.IsCompleted) + { + throw new InvalidOperationException("Expected synchronous disposal."); + } + }); + + await tracker.UntrackObjects(holder); + + await Assert.That(instance.DisposeCount).IsEqualTo(1); + } + + [Test] + public async Task Concurrent_Fetches_In_One_Test_Hold_The_Instance_Once() + { + var key = NewKey(); + var (tracker, _) = CreateTracker(); + var running = CreateContext(); + + running.StartHoldingSharedReferences(); + var fetched = await Task.WhenAll(Enumerable.Range(0, 32).Select(_ => Task.Run(() => + { + using (running.MakeCurrent()) + { + return SharedDataSources.GetOrCreate(SharedType.Keyed, key: key); + } + }))); + + var instance = fetched[0]; + await Assert.That(fetched.All(x => ReferenceEquals(x, instance))).IsTrue(); + + running.StopHoldingSharedReferences(); + await tracker.UntrackObjects(running); + await Assert.That(instance.IsDisposed).IsTrue(); + } + + private static string NewKey() => $"{nameof(SharedInstanceLifetimeTests)}-{Guid.NewGuid():N}"; + + // Fetched by a test that is not running yet, as a data source does at registration. + private static SharedDisposable Fetch(string key) + { + using (CreateContext().MakeCurrent()) + { + return SharedDataSources.GetOrCreate(SharedType.Keyed, key: key); + } + } + + private static (ObjectTracker Tracker, FakeDiscoverer Discoverer) CreateTracker() + { + var discoverer = new FakeDiscoverer(); + return (new ObjectTracker(new TrackableObjectGraphProvider(discoverer), GlobalContext.Current.Disposer), discoverer); + } + + private static SortedList> Add(TestContext context, object obj) + { + var tracked = context.TrackedObjects; + if (!tracked.TryGetValue(0, out var bucket)) + { + bucket = new HashSet(TUnit.Core.Helpers.ReferenceEqualityComparer.Instance); + tracked[0] = bucket; + } + + bucket.Add(obj); + return tracked; + } + + private static TestContext CreateContext() + { + var currentContext = TestContext.Current!; + return new TestContext(nameof(SharedInstanceLifetimeTests), currentContext.ServiceProvider, currentContext.ClassContext, + new TestBuilderContext { TestMetadata = currentContext.TestDetails.MethodMetadata }, CancellationToken.None); + } + + public sealed class SharedDisposable : IDisposable + { + private int _disposeCount; + + public int DisposeCount => Volatile.Read(ref _disposeCount); + + public bool IsDisposed => DisposeCount > 0; + + public void Dispose() => Interlocked.Increment(ref _disposeCount); + } + + private sealed class FakeDiscoverer : IObjectGraphDiscoverer + { + public Func>> Discover { get; set; } = context => context.TrackedObjects; + + public SortedList> DiscoverAndTrackObjects(TestContext testContext, CancellationToken cancellationToken = default) + => Discover(testContext); + + public ObjectGraph DiscoverObjectGraph(TestContext testContext, CancellationToken cancellationToken = default) + => throw new NotSupportedException(); + + public ObjectGraph DiscoverNestedObjectGraph(object rootObject, CancellationToken cancellationToken = default) + => throw new NotSupportedException(); + } +}