From 33a5f57fa14ae946ac19d564c3e50c8ab026364e Mon Sep 17 00:00:00 2001 From: CoreyAlexander Date: Mon, 16 Dec 2024 08:11:34 -0500 Subject: [PATCH 1/3] feat: Add WeakReference to store reference types in a default provider --- .../dependent/DependencyResolver.cs | 50 +++++++++++++++---- .../auto_inject/dependent/DependentState.cs | 2 +- .../src/auto_inject/dependent/IDependent.cs | 2 +- .../test/fixtures/Dependents.cs | 19 +++++++ .../test/src/MiscTest.cs | 2 +- .../test/src/ResolutionTest.cs | 16 +++++- 6 files changed, 77 insertions(+), 14 deletions(-) diff --git a/Chickensoft.AutoInject.Tests/src/auto_inject/dependent/DependencyResolver.cs b/Chickensoft.AutoInject.Tests/src/auto_inject/dependent/DependencyResolver.cs index f7520aa..aa61ef7 100644 --- a/Chickensoft.AutoInject.Tests/src/auto_inject/dependent/DependencyResolver.cs +++ b/Chickensoft.AutoInject.Tests/src/auto_inject/dependent/DependencyResolver.cs @@ -128,8 +128,9 @@ public static TValue DependOn( // First, check dependency fakes. Using a faked value takes priority over // all the other dependency resolution methods. var state = dependent.MixinState.Get(); - if (state.ProviderFakes.TryGetValue(typeof(TValue), out var fakeProvider)) { - return fakeProvider.Value(); + if (state.ProviderFakes.TryGetValue(typeof(TValue), out var fakeProvider) + && fakeProvider is DefaultProvider faker) { + return faker.Value(); } // Lookup dependency, per usual, respecting any fallback values if there @@ -145,15 +146,16 @@ public static TValue DependOn( if (providerNode is IProvide provider) { return provider.Value(); } - else if (providerNode is DefaultProvider defaultProvider) { + else if (providerNode is DefaultProvider defaultProvider) { return defaultProvider.Value(); } } else if (fallback is not null) { // See if we were given a fallback. - var provider = new DefaultProvider(fallback()); + var value = fallback(); + var provider = new DefaultProvider(value, fallback); state.Dependencies.Add(typeof(TValue), provider); - return (TValue)provider.Value(); + return provider.Value(); } throw new ProviderNotFoundException(typeof(TValue)); @@ -283,15 +285,43 @@ void onProviderInitialized(IBaseProvider provider) { // for fallback values. } - public class DefaultProvider : IBaseProvider { - private readonly dynamic _value; + public class DefaultProvider : IBaseProvider { + private object _value; + private readonly Func? _fallback; public ProviderState ProviderState { get; } - public DefaultProvider(dynamic value) { - _value = value; + // When working with reference types, we must wrap the value in a WeakReference<>() + // to allow the garbage collection to work when the assembly is being unloaded or reloaded; + // such as in the case of rebuilding within the Godot Editor if you've instantiated a node + // and run it as a tool script. + public DefaultProvider(object value, Func? fallback = default) { + _fallback = fallback; + + _value = value.GetType().IsValueType ? value : new WeakReference(value); ProviderState = new() { IsInitialized = true }; } - public dynamic Value() => _value; + public TValue Value() { + if (_value is WeakReference weakReference) { + // Try to return a reference type. + if (weakReference.TryGetTarget(out var target)) { + return (TValue)target; + } + else { + // If value was garbage collected, make a new one. + if (_fallback == null) { + throw new InvalidOperationException("Fallback method is null."); + } + + var value = _fallback() ?? throw new InvalidOperationException("Fallback cannot create a null value"); + _value = new WeakReference(value); + return value; + } + } + else { + // Return a value type. + return (TValue)_value; + } + } } } diff --git a/Chickensoft.AutoInject.Tests/src/auto_inject/dependent/DependentState.cs b/Chickensoft.AutoInject.Tests/src/auto_inject/dependent/DependentState.cs index 2d746dc..2b3e8bf 100644 --- a/Chickensoft.AutoInject.Tests/src/auto_inject/dependent/DependentState.cs +++ b/Chickensoft.AutoInject.Tests/src/auto_inject/dependent/DependentState.cs @@ -51,7 +51,7 @@ public class DependentState { /// allows nodes being unit-tested to provide fake providers during unit tests /// that return mock or faked values. /// - public Dictionary ProviderFakes { + public DependencyResolver.DependencyTable ProviderFakes { get; } = []; } diff --git a/Chickensoft.AutoInject.Tests/src/auto_inject/dependent/IDependent.cs b/Chickensoft.AutoInject.Tests/src/auto_inject/dependent/IDependent.cs index f34c19f..29774f1 100644 --- a/Chickensoft.AutoInject.Tests/src/auto_inject/dependent/IDependent.cs +++ b/Chickensoft.AutoInject.Tests/src/auto_inject/dependent/IDependent.cs @@ -98,6 +98,6 @@ this is IIntrospective introspective && public void FakeDependency(T value) where T : notnull { AddStateIfNeeded(); MixinState.Get().ProviderFakes[typeof(T)] = - new DependencyResolver.DefaultProvider(value); + new DependencyResolver.DefaultProvider(value); } } diff --git a/Chickensoft.AutoInject.Tests/test/fixtures/Dependents.cs b/Chickensoft.AutoInject.Tests/test/fixtures/Dependents.cs index 589b7bd..51d96b8 100644 --- a/Chickensoft.AutoInject.Tests/test/fixtures/Dependents.cs +++ b/Chickensoft.AutoInject.Tests/test/fixtures/Dependents.cs @@ -56,6 +56,25 @@ public void OnResolved() { } } +[Meta(typeof(IAutoOn), typeof(IDependent))] +public partial class ReferenceDependentFallback : Node { + public override void _Notification(int what) => this.Notify(what); + + [Dependency] + public object MyDependency => this.DependOn(() => FallbackValue); + + public object FallbackValue { get; set; } = new Resource(); + public bool OnResolvedCalled { get; private set; } + public object ResolvedValue { get; set; } = null!; + + public void OnReady() { } + + public void OnResolved() { + OnResolvedCalled = true; + ResolvedValue = MyDependency; + } +} + [Meta(typeof(IAutoOn), typeof(IDependent))] public partial class IntDependent : Node { public override void _Notification(int what) => this.Notify(what); diff --git a/Chickensoft.AutoInject.Tests/test/src/MiscTest.cs b/Chickensoft.AutoInject.Tests/test/src/MiscTest.cs index 58223a9..7ebf576 100644 --- a/Chickensoft.AutoInject.Tests/test/src/MiscTest.cs +++ b/Chickensoft.AutoInject.Tests/test/src/MiscTest.cs @@ -47,7 +47,7 @@ public void IDependentOnResolvedDoesNothing() { [Test] public void DefaultProviderState() { - var defaultProvider = new DependencyResolver.DefaultProvider("hi"); + var defaultProvider = new DependencyResolver.DefaultProvider("hi"); defaultProvider.ProviderState.ShouldNotBeNull(); } } diff --git a/Chickensoft.AutoInject.Tests/test/src/ResolutionTest.cs b/Chickensoft.AutoInject.Tests/test/src/ResolutionTest.cs index b5e825f..50185e7 100644 --- a/Chickensoft.AutoInject.Tests/test/src/ResolutionTest.cs +++ b/Chickensoft.AutoInject.Tests/test/src/ResolutionTest.cs @@ -131,7 +131,7 @@ public void ThrowsWhenNoProviderFound() { } [Test] - public void UsesFallbackValueWhenNoProviderFound() { + public void UsesValueFallbackValueWhenNoProviderFound() { var fallback = "Hello, world!"; var dependent = new StringDependentFallback { FallbackValue = fallback @@ -142,6 +142,20 @@ public void UsesFallbackValueWhenNoProviderFound() { dependent.ResolvedValue.ShouldBe(fallback); dependent.MyDependency.ShouldBe(fallback); } + + [Test] + public void UsesReferenceFallbackValueWhenNoProviderFound() { + var fallback = new Resource(); + var dependent = new ReferenceDependentFallback { + FallbackValue = fallback + }; + + dependent._Notification((int)Node.NotificationReady); + + dependent.ResolvedValue.ShouldBe(fallback); + dependent.MyDependency.ShouldBe(fallback); + } + [Test] public void ThrowsOnDependencyTableThatWasTamperedWith() { var fallback = "Hello, world!"; From b07e306d24d032c8a0fa95405f9fe90f234a46f8 Mon Sep 17 00:00:00 2001 From: Joanna May Date: Sat, 21 Dec 2024 12:45:41 -0600 Subject: [PATCH 2/3] test: gc scenarios --- Chickensoft.AutoInject.Tests/coverage.sh | 2 +- .../dependent/DependencyResolver.cs | 50 +++++----- .../test/fixtures/Dependents.cs | 19 +++- .../test/src/ResolutionTest.cs | 94 +++++++++++++++++-- cspell.json | 7 +- 5 files changed, 133 insertions(+), 39 deletions(-) diff --git a/Chickensoft.AutoInject.Tests/coverage.sh b/Chickensoft.AutoInject.Tests/coverage.sh index e9fb87e..4cd7581 100755 --- a/Chickensoft.AutoInject.Tests/coverage.sh +++ b/Chickensoft.AutoInject.Tests/coverage.sh @@ -26,7 +26,7 @@ dotnet build --no-restore coverlet \ "./.godot/mono/temp/bin/Debug" --verbosity detailed \ - --target $GODOT \ + --target "$GODOT" \ --targetargs "--headless --run-tests --coverage --quit-on-finish" \ --format "opencover" \ --output "./coverage/coverage.xml" \ diff --git a/Chickensoft.AutoInject.Tests/src/auto_inject/dependent/DependencyResolver.cs b/Chickensoft.AutoInject.Tests/src/auto_inject/dependent/DependencyResolver.cs index aa61ef7..f80cd6d 100644 --- a/Chickensoft.AutoInject.Tests/src/auto_inject/dependent/DependencyResolver.cs +++ b/Chickensoft.AutoInject.Tests/src/auto_inject/dependent/DependencyResolver.cs @@ -286,42 +286,44 @@ void onProviderInitialized(IBaseProvider provider) { } public class DefaultProvider : IBaseProvider { - private object _value; - private readonly Func? _fallback; + internal object _value; + private readonly Func _fallback; public ProviderState ProviderState { get; } - // When working with reference types, we must wrap the value in a WeakReference<>() - // to allow the garbage collection to work when the assembly is being unloaded or reloaded; - // such as in the case of rebuilding within the Godot Editor if you've instantiated a node + // When working with reference types, we must wrap the value in a + // WeakReference() to allow the garbage collection to work when the + // assembly is being unloaded or reloaded; such as in the case of + // rebuilding within the Godot Editor if you've instantiated a node // and run it as a tool script. - public DefaultProvider(object value, Func? fallback = default) { - _fallback = fallback; + public DefaultProvider(object value, Func? fallback = default) { + _fallback = fallback ?? (() => (TValue?)value); + + _value = value.GetType().IsValueType + ? value + : new WeakReference(value); - _value = value.GetType().IsValueType ? value : new WeakReference(value); ProviderState = new() { IsInitialized = true }; } public TValue Value() { - if (_value is WeakReference weakReference) { + if (_value is WeakReference weakReference) { // Try to return a reference type. - if (weakReference.TryGetTarget(out var target)) { - return (TValue)target; + if (weakReference.Target is TValue target) { + return target; } - else { - // If value was garbage collected, make a new one. - if (_fallback == null) { - throw new InvalidOperationException("Fallback method is null."); - } - var value = _fallback() ?? throw new InvalidOperationException("Fallback cannot create a null value"); - _value = new WeakReference(value); - return value; - } - } - else { - // Return a value type. - return (TValue)_value; + var value = _fallback() ?? + throw new InvalidOperationException( + "Fallback cannot create a null value" + ); + + _value = new WeakReference(value); + + return value; + } + // Return a value type. + return (TValue)_value; } } } diff --git a/Chickensoft.AutoInject.Tests/test/fixtures/Dependents.cs b/Chickensoft.AutoInject.Tests/test/fixtures/Dependents.cs index 51d96b8..6c1d7f8 100644 --- a/Chickensoft.AutoInject.Tests/test/fixtures/Dependents.cs +++ b/Chickensoft.AutoInject.Tests/test/fixtures/Dependents.cs @@ -1,5 +1,6 @@ namespace Chickensoft.AutoInject.Tests.Subjects; +using System; using Chickensoft.Introspection; using Godot; @@ -56,6 +57,21 @@ public void OnResolved() { } } +[Meta(typeof(IAutoOn), typeof(IDependent))] +public partial class WeakReferenceDependent : Node { + public override void _Notification(int what) => this.Notify(what); + + [Dependency] + public object MyDependency => this.DependOn(Fallback); + + public Func? Fallback { get; set; } + public bool OnResolvedCalled { get; private set; } + + public void OnReady() { } + + public void OnResolved() => OnResolvedCalled = true; +} + [Meta(typeof(IAutoOn), typeof(IDependent))] public partial class ReferenceDependentFallback : Node { public override void _Notification(int what) => this.Notify(what); @@ -80,10 +96,11 @@ public partial class IntDependent : Node { public override void _Notification(int what) => this.Notify(what); [Dependency] - public int MyDependency => this.DependOn(); + public int MyDependency => this.DependOn(FallbackValue); public bool OnResolvedCalled { get; private set; } public int ResolvedValue { get; set; } + public Func? FallbackValue { get; set; } public void OnReady() { } diff --git a/Chickensoft.AutoInject.Tests/test/src/ResolutionTest.cs b/Chickensoft.AutoInject.Tests/test/src/ResolutionTest.cs index 50185e7..fceacaf 100644 --- a/Chickensoft.AutoInject.Tests/test/src/ResolutionTest.cs +++ b/Chickensoft.AutoInject.Tests/test/src/ResolutionTest.cs @@ -1,5 +1,7 @@ namespace Chickensoft.AutoInject.Tests; +using System; +using System.Collections.Generic; using System.Threading.Tasks; using Chickensoft.AutoInject.Tests.Subjects; using Chickensoft.GoDotTest; @@ -131,9 +133,9 @@ public void ThrowsWhenNoProviderFound() { } [Test] - public void UsesValueFallbackValueWhenNoProviderFound() { - var fallback = "Hello, world!"; - var dependent = new StringDependentFallback { + public void UsesReferenceFallbackValueWhenNoProviderFound() { + var fallback = new Resource(); + var dependent = new ReferenceDependentFallback { FallbackValue = fallback }; @@ -144,16 +146,73 @@ public void UsesValueFallbackValueWhenNoProviderFound() { } [Test] - public void UsesReferenceFallbackValueWhenNoProviderFound() { - var fallback = new Resource(); - var dependent = new ReferenceDependentFallback { - FallbackValue = fallback + public void DependsOnValueType() { + var value = 10; + var depObj = new IntDependent() { FallbackValue = () => value }; + var dependent = depObj as IDependent; + + depObj._Notification((int)Node.NotificationReady); + + + depObj.OnResolvedCalled.ShouldBeTrue(); + depObj.ResolvedValue.ShouldBe(value); + + depObj._Notification((int)Node.NotificationExitTree); + + dependent.DependentState.Pending.ShouldBeEmpty(); + + depObj.QueueFree(); + } + + [Test] + public void ThrowsIfFallbackProducesNullAfterPreviousValueIsGarbageCollected( + ) { + var currentFallback = 0; + var replacementValue = new object(); + var fallbacks = new List() { new(), null, replacementValue }; + + var value = Utils.CreateWeakReference(); + + // Fallback will be called 3 times in this test. First will be non-null, + // second will be null, third will be non-null and different from the first. + var depObj = new WeakReferenceDependent() { + Fallback = () => fallbacks[currentFallback++]! }; - dependent._Notification((int)Node.NotificationReady); + var dependent = depObj as IDependent; - dependent.ResolvedValue.ShouldBe(fallback); - dependent.MyDependency.ShouldBe(fallback); + depObj._Notification((int)Node.NotificationReady); + + // Let's access the fallback value to ensure the default provider is setup. + depObj.MyDependency.ShouldNotBeNull(); + + // Simulate a garbage collected object. We support weak references to + // dependencies to avoid causing build issues when reloading the scene. + Utils.ClearWeakReference(value); + + // To test this highly specific scenario, we have to clear ALL + // weak references to the object, including the one in the default provider + // that's generated behind-the-scenes for us. + + // Let's dig out the weak ref used in the default provider from the + // dependent's internal state... + var underlyingDefaultProvider = + (DependencyResolver.DefaultProvider) + depObj.MixinState.Get().Dependencies[typeof(object)]; + + var actualWeakRef = (WeakReference)underlyingDefaultProvider._value; + + Utils.ClearWeakReference(actualWeakRef); + + var e = Should.Throw( + () => depObj.MyDependency + ); + + e.Message.ShouldContain("cannot create a null value"); + + // Now that the fallback returns a valid value, the dependency should + // be resolved once again. + depObj.MyDependency.ShouldBeSameAs(replacementValue); } [Test] @@ -246,4 +305,19 @@ public BadProvider() { }; } } + + public static class Utils { + public static WeakReference CreateWeakReference(Func? factory = null) => new( + factory?.Invoke() ?? new object() + ); + + public static void ClearWeakReference(WeakReference weakReference) { + weakReference.Target = null; + + while (weakReference.Target is not null) { + GC.Collect(); + GC.WaitForPendingFinalizers(); + } + } + } } diff --git a/cspell.json b/cspell.json index 4f5e4dd..518c935 100644 --- a/cspell.json +++ b/cspell.json @@ -13,9 +13,6 @@ "Chickensoft.AutoInject/nupkg/**/*.*" ], "words": [ - "devbuild", - "mktemp", - "skipautoprops", "assemblyfilters", "automerge", "branchcoverage", @@ -25,7 +22,9 @@ "Chickensoft", "classfilters", "CYGWIN", + "devbuild", "endregion", + "Finalizers", "globaltool", "godotengine", "godotpackage", @@ -35,6 +34,7 @@ "Metatype", "methodcoverage", "missingall", + "mktemp", "msbuild", "MSYS", "nameof", @@ -52,6 +52,7 @@ "reportgenerator", "reporttypes", "Shouldly", + "skipautoprops", "subfolders", "targetargs", "targetdir", From a641cade051044cff2482e73924d703ef641508e Mon Sep 17 00:00:00 2001 From: Joanna May Date: Sat, 21 Dec 2024 12:52:15 -0600 Subject: [PATCH 3/3] fix: simplify test --- Chickensoft.AutoInject.Tests/test/src/ResolutionTest.cs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/Chickensoft.AutoInject.Tests/test/src/ResolutionTest.cs b/Chickensoft.AutoInject.Tests/test/src/ResolutionTest.cs index fceacaf..9c3c487 100644 --- a/Chickensoft.AutoInject.Tests/test/src/ResolutionTest.cs +++ b/Chickensoft.AutoInject.Tests/test/src/ResolutionTest.cs @@ -307,9 +307,7 @@ public BadProvider() { } public static class Utils { - public static WeakReference CreateWeakReference(Func? factory = null) => new( - factory?.Invoke() ?? new object() - ); + public static WeakReference CreateWeakReference() => new(new object()); public static void ClearWeakReference(WeakReference weakReference) { weakReference.Target = null;