From 5b653f6c06f285055ceca715ac550290cd360c84 Mon Sep 17 00:00:00 2001 From: dadhi Date: Tue, 2 Aug 2022 17:05:15 +0200 Subject: [PATCH] fixed: #507; improving the docs for resolution scope disposal --- docs/DryIoc.Docs/ReuseAndScopes.cs | 29 ++++-- docs/DryIoc.Docs/ReuseAndScopes.md | 29 ++++-- src/DryIoc/Container.Generated.cs | 8 +- src/DryIoc/Container.cs | 25 +++++- ...g_scope_using_factory_func_in_singleton.cs | 4 +- ...107_NamedScopesDependingOnResolvedTypes.cs | 89 ++++++++++++++++--- test/DryIoc.TestRunner/Program.cs | 1 + 7 files changed, 146 insertions(+), 39 deletions(-) diff --git a/docs/DryIoc.Docs/ReuseAndScopes.cs b/docs/DryIoc.Docs/ReuseAndScopes.cs index 12c5b1151..f6aa1c1c9 100644 --- a/docs/DryIoc.Docs/ReuseAndScopes.cs +++ b/docs/DryIoc.Docs/ReuseAndScopes.cs @@ -18,7 +18,9 @@ - [Reuse.ScopedTo(name)](#reusescopedtoname) - [Reuse.InWebRequest and Reuse.InThread](#reuseinwebrequest-and-reuseinthread) - [Reuse.ScopedTo service type](#reusescopedto-service-type) - - [Own the resolution scope disposal](#own-the-resolution-scope-disposal) + - [Disposing of resolution scope](#disposing-of-resolution-scope) + - [Automatic scope disposal](#automatic-scope-disposal) + - [Own the resolution scope disposal](#own-the-resolution-scope-disposal) - [Setup.UseParentReuse](#setupuseparentreuse) - [Reuse lifespan diagnostics](#reuse-lifespan-diagnostics) - [Weakly Referenced reused service](#weakly-referenced-reused-service) @@ -660,11 +662,18 @@ To satisfy the `ScopedTo` reuse we need an open scope somewhere. Here DryIoc wil **Note:** The code also tells that `Foo` itself will be scoped to its scope. It may be important if you want to access `Foo` recursively from its dependency. -But with a such an automatic scoping we still have a problem: how to dispose the scope. -In order to dispose the scope, DryIoc should somehow track the reference to it - **otherwise we are in the memory leak territory**. +### Disposing of resolution scope -This is exactly how it works, the new resolution scope will itself be held by either the upper scope or by the singleton scope. When the upper scope or container with singletons is disposed - the resolutions scope is disposed to. +#### Automatic scope disposal + +Having such an implicit opened scope poses a problem - how to dispose the scope? + +In order to dispose the scope DryIoc or the user code (explained later) +should track the reference to it otherwise we have an undisposed dangling scope - **which is the bad thing to have**. + +Therefore to avoid dangling scope the resolution scope will be automatically tracked by either the parent scope or by the singleton scope. +When the parent scope or container with singletons is disposed - the resolutions scope is disposed too. ```cs md*/ class Scoped_to_service_reuse_with_dispose @@ -697,10 +706,14 @@ class Dependency : IDisposable } /*md ``` -### Own the resolution scope disposal -You else may get in control of disposing the resolution scope by injecting the `IResolverContext` ([automatically provided for you by Container](RulesAndDefaultConventions.md#implicitly-available-services)) -which holds the current scope and then dispose it manually. +#### Own the resolution scope disposal + +The scope tracking is not ideal because it postpones the disposal until parent container or parent scope is disposed +But we may want to dispose it sooner, e.g. together with the service that opened it. + +You may control the disposing of the resolution scope by injecting its `IResolverContext` ([automatically provided by Container](RulesAndDefaultConventions.md#implicitly-available-services)) +and then dispose it manually. ```cs md*/ class Own_the_resolution_scope_disposal @@ -717,7 +730,7 @@ [Test] public void Example() var foo = container.Resolve(); - // Disposing the foo will dispose its scope and the scoped dependencies down the tree + // Disposing the foo will dispose its scope and its scoped dependencies down the tree foo.Dispose(); Assert.IsTrue(foo.Dep.IsDisposed); diff --git a/docs/DryIoc.Docs/ReuseAndScopes.md b/docs/DryIoc.Docs/ReuseAndScopes.md index 5aa9e1760..0c5a8f65b 100644 --- a/docs/DryIoc.Docs/ReuseAndScopes.md +++ b/docs/DryIoc.Docs/ReuseAndScopes.md @@ -17,7 +17,9 @@ - [Reuse.ScopedTo(name)](#reusescopedtoname) - [Reuse.InWebRequest and Reuse.InThread](#reuseinwebrequest-and-reuseinthread) - [Reuse.ScopedTo service type](#reusescopedto-service-type) - - [Own the resolution scope disposal](#own-the-resolution-scope-disposal) + - [Disposing of resolution scope](#disposing-of-resolution-scope) + - [Automatic scope disposal](#automatic-scope-disposal) + - [Own the resolution scope disposal](#own-the-resolution-scope-disposal) - [Setup.UseParentReuse](#setupuseparentreuse) - [Reuse lifespan diagnostics](#reuse-lifespan-diagnostics) - [Weakly Referenced reused service](#weakly-referenced-reused-service) @@ -656,11 +658,18 @@ var foo container.OpenScope(new ResolutionScopeName(typeof(Foo))).Resolve() **Note:** The code also tells that `Foo` itself will be scoped to its scope. It may be important if you want to access `Foo` recursively from its dependency. -But with a such an automatic scoping we still have a problem: how to dispose the scope. -In order to dispose the scope, DryIoc should somehow track the reference to it - **otherwise we are in the memory leak territory**. +### Disposing of resolution scope -This is exactly how it works, the new resolution scope will itself be held by either the upper scope or by the singleton scope. When the upper scope or container with singletons is disposed - the resolutions scope is disposed to. +#### Automatic scope disposal + +Having such an implicit opened scope poses a problem - how to dispose the scope? + +In order to dispose the scope DryIoc or the user code (explained later) +should track the reference to it otherwise we have an undisposed dangling scope - **which is the bad thing to have**. + +Therefore to avoid dangling scope the resolution scope will be automatically tracked by either the parent scope or by the singleton scope. +When the parent scope or container with singletons is disposed - the resolutions scope is disposed to0. ```cs class Scoped_to_service_reuse_with_dispose @@ -693,10 +702,14 @@ class Scoped_to_service_reuse_with_dispose } ``` -### Own the resolution scope disposal -You else may get in control of disposing the resolution scope by injecting the `IResolverContext` ([automatically provided for you by Container](RulesAndDefaultConventions.md#implicitly-available-services)) -which holds the current scope and then dispose it manually. +#### Own the resolution scope disposal + +The scope tracking is not ideal because it postpones the disposal until parent container or parent scope is disposed +But we may want to dispose it sooner, e.g. together with the service that opened it. + +You may control the disposing of the resolution scope by injecting its `IResolverContext` ([automatically provided by Container](RulesAndDefaultConventions.md#implicitly-available-services)) +and then dispose it manually. ```cs class Own_the_resolution_scope_disposal @@ -713,7 +726,7 @@ class Own_the_resolution_scope_disposal var foo = container.Resolve(); - // Disposing the foo will dispose its scope and the scoped dependencies down the tree + // Disposing the foo will dispose its scope and its scoped dependencies down the tree foo.Dispose(); Assert.IsTrue(foo.Dep.IsDisposed); diff --git a/src/DryIoc/Container.Generated.cs b/src/DryIoc/Container.Generated.cs index bd040c202..a176efed0 100644 --- a/src/DryIoc/Container.Generated.cs +++ b/src/DryIoc/Container.Generated.cs @@ -44,7 +44,7 @@ partial class Container { partial void GetLastGeneratedFactoryID(ref int lastFactoryID) { - lastFactoryID = 261; // generated: equals to the last used Factory.FactoryID + lastFactoryID = 352; // generated: equals to the last used Factory.FactoryID } partial void ResolveGenerated(ref object service, Type serviceType) @@ -62,7 +62,7 @@ partial void ResolveGenerated(ref object service, requiredServiceType == null && Equals(preRequestParent, Request.Empty.Push( typeof(IService), - 256, + 347, typeof(MyService), Reuse.Transient, RequestFlags.IsResolutionCall))) @@ -92,7 +92,7 @@ internal static object Get_IService_0(IResolverContext r) => null, Request.Empty.Push( typeof(IService), - 256, + 347, typeof(MyService), Reuse.Transient, RequestFlags.IsResolutionCall|RequestFlags.StopRecursiveDependencyCheck), @@ -104,7 +104,7 @@ internal static object Get_IService_0(IResolverContext r) => null, Request.Empty.Push( typeof(IService), - 256, + 347, typeof(MyService), Reuse.Transient, RequestFlags.IsResolutionCall|RequestFlags.StopRecursiveDependencyCheck), diff --git a/src/DryIoc/Container.cs b/src/DryIoc/Container.cs index 3177746e7..c82231db6 100644 --- a/src/DryIoc/Container.cs +++ b/src/DryIoc/Container.cs @@ -4657,11 +4657,24 @@ public static class ResolverContext public static Expression GetRootOrSelfExpr(Request request) => request.Reuse is CurrentScopeReuse == false && request.DirectParent.IsSingletonOrDependencyOfSingleton - && !request.OpensResolutionScope && request.Rules.ThrowIfDependencyHasShorterReuseLifespan // see the #378 + // && !request.OpensResolutionScope + && !request.OpensResolutionScopeUpToResolutionCall() ? RootOrSelfExpr : FactoryDelegateCompiler.ResolverContextParamExpr; + private static bool OpensResolutionScopeUpToResolutionCall(this Request r) + { + var p = r.DirectParent; + while (p != null) + { + if ((p.Flags & RequestFlags.OpensResolutionScope) != 0) + return true; + p = p.DirectParent; + } + return false; + } + /// Root or the current resolver context (if it is the root). public static readonly Expression RootOrSelfExpr = new ResolverContextArgMethodCallExpression(typeof(ResolverContext).GetMethod(nameof(RootOrSelf))); @@ -9386,6 +9399,8 @@ public static Request Create(IContainer container, ServiceInfo serviceInfo, flags |= preResolveParent.Flags & InheritedFlags; } + else + flags |= preResolveParent.Flags; //inherits the OpensResolutionScope flag var inputArgExprs = inputArgs?.Map(a => Constant(a)); // todo: @check what happens if `a == null`, does the `object` type for is fine @@ -9491,16 +9506,16 @@ internal void DecreaseTrackedDependencyCountForParents(int dependencyCount) public bool IsEmpty => DirectParent == null; /// Returns true if request is First in First Resolve call. - public bool IsResolutionRoot => !IsEmpty && DirectParent.IsEmpty; + public bool IsResolutionRoot => !IsEmpty && DirectParent.IsEmpty; // todo: @perf remove check for empty /// Returns true if request is First in Resolve call. - public bool IsResolutionCall => !IsEmpty && (Flags & RequestFlags.IsResolutionCall) != 0; + public bool IsResolutionCall => !IsEmpty && (Flags & RequestFlags.IsResolutionCall) != 0; // todo: @perf remove check for empty /// Not the root resolution call. public bool IsNestedResolutionCall => IsResolutionCall && !DirectParent.IsEmpty; /// Returns true if request is First in First Resolve call. - public bool OpensResolutionScope => !IsEmpty && (DirectParent.Flags & RequestFlags.OpensResolutionScope) != 0; + public bool OpensResolutionScope => !IsEmpty && (DirectParent.Flags & RequestFlags.OpensResolutionScope) != 0; // todo: @perf remove check for empty /// Checks if the request Or its parent is wrapped in Func. Use `IsDirectlyWrappedInFunc` for the direct Func wrapper. public bool IsWrappedInFunc() => (Flags & RequestFlags.IsWrappedInFunc) != 0; @@ -10824,6 +10839,8 @@ cacheEntry.Value is Container.ExprCacheOfTransientWithDepCount t && t.Count > 0 } // At last, create the object graph with all of the dependencies created and injected + // todo: @perf optimize the code path for IResolverContext Wrapper: ResolverContext.GetRootOrSelfExpr - override GetExpressionOrDefault + // todo: @perf for IResolverContext no need to check expression cache at all serviceExpr = serviceFactory == null ? CreateExpressionOrDefault(request) : CreateExpressionWithWrappedFactory(request, serviceFactory); diff --git a/test/DryIoc.IssuesTests/GHIssue507_Transient_resolve_with_opening_scope_using_factory_func_in_singleton.cs b/test/DryIoc.IssuesTests/GHIssue507_Transient_resolve_with_opening_scope_using_factory_func_in_singleton.cs index c946fd41b..24e4a6a9c 100644 --- a/test/DryIoc.IssuesTests/GHIssue507_Transient_resolve_with_opening_scope_using_factory_func_in_singleton.cs +++ b/test/DryIoc.IssuesTests/GHIssue507_Transient_resolve_with_opening_scope_using_factory_func_in_singleton.cs @@ -9,7 +9,7 @@ public class GHIssue507_Transient_resolve_with_opening_scope_using_factory_func_ public int Run() { Test_Simple(); - // Test_Original_issue(); + Test_Original_issue(); return 2; } @@ -41,7 +41,7 @@ class Foo2 { } - [Test, Ignore("fixme")] + [Test] public void Test_Original_issue() { var container = new Container(rules => rules.WithTrackingDisposableTransients()); diff --git a/test/DryIoc.IssuesTests/Issue107_NamedScopesDependingOnResolvedTypes.cs b/test/DryIoc.IssuesTests/Issue107_NamedScopesDependingOnResolvedTypes.cs index f2b660b30..634aef78e 100644 --- a/test/DryIoc.IssuesTests/Issue107_NamedScopesDependingOnResolvedTypes.cs +++ b/test/DryIoc.IssuesTests/Issue107_NamedScopesDependingOnResolvedTypes.cs @@ -6,8 +6,21 @@ namespace DryIoc.IssuesTests { [TestFixture] - public class Issue107_NamedScopesDependingOnResolvedTypes + public class Issue107_NamedScopesDependingOnResolvedTypes : ITest { + public int Run() + { + Can_reuse_and_locate_based_on_object_graph_itself(); + Can_reuse_based_on_object_graph_itself(); + Achievable_with_dynamic_dependency_and_resolution_scope(); + Service_scoped_to_the_correct_resolution_scope_should_be_disposed_even_for_root_singleton_with_resolution_scope(); + Service_scoped_to_the_correct_resolution_scope_should_be_disposed_even_for_root_singleton_with_resolution_scope_2(); + Service_scoped_to_resolve_dependency_should_be_disposed_simplified(); + SingletonWithSeveralInterfaces_ResolveEachInterface_EachResolveReturnsSameInstance(); + Can_inject_property_of_larger_interface_without_register_delegate(); + return 8; + } + public interface ITwoVariants { } internal class FirstVariant : ITwoVariants { } internal class SecondVariant : ITwoVariants { } @@ -251,7 +264,7 @@ public void Can_reuse_and_locate_based_on_object_graph_itself() Assert.AreSame(component.Area1.Database, component.Area1.MainViewModel1.WithChildren.Simple.Database, "Inside of area always the same database"); Assert.AreNotSame(component.Area1.Database, component.Area2.Database, "Each area with own database"); - // ViewModelPrsenter (LifestyleBoundToNearest): Same in Area1 and Area 2 + // ViewModelPresenter (LifestyleBoundToNearest): Same in Area1 and Area 2 Assert.AreSame(component.Area1.MainViewModel1.ViewModelPresenter, component.Area1.MainViewModel1.Simple.ViewModelPresenter, "All ViewModelChildren shares with the owning MainViewModel same ViewModelPresenter"); Assert.AreSame(component.Area1.MainViewModel1.ViewModelPresenter, component.Area1.MainViewModel1.WithChildren.ViewModelPresenter, "All ViewModelChildren shares with the owning MainViewModel same ViewModelPresenter"); Assert.AreSame(component.Area1.MainViewModel1.ViewModelPresenter, component.Area1.MainViewModel1.WithChildren.Simple.ViewModelPresenter, "All ViewModelChildren shares with the owning MainViewModel same ViewModelPresenter"); @@ -440,35 +453,83 @@ public CarefulAreaManagerSimplified(AreaWithOneCarSimplified area) } } - internal class CarefulAreaManager : IDisposable + internal class NotCarefulAreaManager : IDisposable { public AreaWithOneCar[] Areas { get; private set; } public ICar ReferenceCar { get; private set; } - public CarefulAreaManager(AreaWithOneCar[] areas, ICar referenceCar) + public NotCarefulAreaManager(AreaWithOneCar[] areas, ICar referenceCar) + { + Areas = areas; + ReferenceCar = referenceCar; + } + + public void Dispose() + { + foreach (var area in Areas) + area.Dispose(); + } + } + + internal class CarefulAreaManager : IDisposable + { + public AreaWithOneCar[] Areas { get; private set; } + public ICar ReferenceCar { get; private set; } + private IResolverContext _resolutionScope; + public CarefulAreaManager(AreaWithOneCar[] areas, ICar referenceCar, IResolverContext resolutionScope) { Areas = areas; ReferenceCar = referenceCar; + _resolutionScope = resolutionScope; } public void Dispose() { foreach (var area in Areas) area.Dispose(); + + _resolutionScope?.Dispose(); + _resolutionScope = null; } } [Test] - public void Service_scoped_to_resolve_dependency_should_be_disposed() + public void Service_scoped_to_the_correct_resolution_scope_should_be_disposed_even_for_root_singleton_with_resolution_scope() { var container = new Container(); container.Register(Reuse.Scoped); - container.Register(Reuse.Scoped, - setup: Setup.With(openResolutionScope: true)); + container.Register(Reuse.Scoped, setup: Setup.With(openResolutionScope: true)); container.Register(); - container.Register(Reuse.Singleton, - setup: Setup.With(openResolutionScope: true)); + container.Register(Reuse.Singleton, setup: Setup.With(openResolutionScope: true)); + + var manager = container.Resolve(); + var area = manager.Areas.First(); + + Assert.AreSame(area.Car, area.Tool.Car); + Assert.AreNotSame(manager.ReferenceCar, area.Car); + + manager.Dispose(); + + Assert.IsTrue(((SmallCar)area.Car).IsDisposed); + Assert.IsTrue(((SmallCar)area.Tool.Car).IsDisposed); + + // The manager open resolution scope is tracked in the singleton scope (because no other scope available), + // therefore the Scope SmallCar injected into manager will be disposed only with singletons/container dispose. + // Alternatively, you may inject the scoped IResolverContext into manager and dispose of it in manager Dispose. + container.Dispose(); + Assert.IsTrue(((SmallCar)manager.ReferenceCar).IsDisposed); + } + + [Test] + public void Service_scoped_to_the_correct_resolution_scope_should_be_disposed_even_for_root_singleton_with_resolution_scope_2() + { + var container = new Container(); + + container.Register(Reuse.Scoped); + container.Register(Reuse.Scoped, setup: Setup.With(openResolutionScope: true)); + container.Register(); + container.Register(Reuse.Singleton, setup: Setup.With(openResolutionScope: true)); var manager = container.Resolve(); var area = manager.Areas.First(); @@ -476,10 +537,14 @@ public void Service_scoped_to_resolve_dependency_should_be_disposed() Assert.AreSame(area.Car, area.Tool.Car); Assert.AreNotSame(manager.ReferenceCar, area.Car); + // Alternatively, you may inject the scoped IResolverContext into manager and dispose of it in manager Dispose. manager.Dispose(); Assert.IsTrue(((SmallCar)area.Car).IsDisposed); Assert.IsTrue(((SmallCar)area.Tool.Car).IsDisposed); + + // does not require dispose because our manager is careful to inject its own resolutions scope and dispose of it. + // container.Dispose(); Assert.IsTrue(((SmallCar)manager.ReferenceCar).IsDisposed); } @@ -488,11 +553,9 @@ public void Service_scoped_to_resolve_dependency_should_be_disposed_simplified() { var container = new Container(); - container.Register(Reuse.Scoped, - setup: Setup.With(openResolutionScope: true)); + container.Register(Reuse.Scoped, setup: Setup.With(openResolutionScope: true)); - container.Register(Reuse.Singleton, - setup: Setup.With(openResolutionScope: true)); + container.Register(Reuse.Singleton, setup: Setup.With(openResolutionScope: true)); var manager = container.Resolve(); diff --git a/test/DryIoc.TestRunner/Program.cs b/test/DryIoc.TestRunner/Program.cs index 6c0ac686c..6915fc2ea 100644 --- a/test/DryIoc.TestRunner/Program.cs +++ b/test/DryIoc.TestRunner/Program.cs @@ -46,6 +46,7 @@ void Run(Func run, string name = null) new OpenGenericsTests(), new DynamicRegistrationsTests(), new SelectConstructorWithAllResolvableArgumentTests(), + new Issue107_NamedScopesDependingOnResolvedTypes(), new GHIssue378_InconsistentResolutionFailure(), new GHIssue380_ExportFactory_throws_Container_disposed_exception(), new GHIssue391_Deadlock_during_Resolve(),