From 7c3a945d012336205078d28be6df25f17fb5e73d Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 8 Aug 2019 14:53:17 +1000 Subject: [PATCH] Fixes DI Lifetime.Request works per thread. #6044 --- src/Umbraco.Core/Composing/Lifetime.cs | 15 ++++----- .../LightInject/LightInjectContainer.cs | 20 +++++++----- src/Umbraco.Core/Umbraco.Core.csproj | 2 +- src/Umbraco.Tests/Umbraco.Tests.csproj | 2 +- .../LightInject/LightInjectContainer.cs | 31 +++++++++++++++++++ src/Umbraco.Web/Umbraco.Web.csproj | 2 +- 6 files changed, 53 insertions(+), 19 deletions(-) diff --git a/src/Umbraco.Core/Composing/Lifetime.cs b/src/Umbraco.Core/Composing/Lifetime.cs index 012555be5e..4577ff06b4 100644 --- a/src/Umbraco.Core/Composing/Lifetime.cs +++ b/src/Umbraco.Core/Composing/Lifetime.cs @@ -1,4 +1,6 @@ -namespace Umbraco.Core.Composing +using System; + +namespace Umbraco.Core.Composing { /// /// Specifies the lifetime of a registered instance. @@ -12,13 +14,6 @@ /// or MS.DI, PerDependency in Autofac. Transient, - // TODO: We need to fix this up, currently LightInject is the only one that behaves differently from all other containers. - // ... the simple fix would be to map this to PerScopeLifetime in LI but need to wait on a response here https://github.com/seesharper/LightInject/issues/494#issuecomment-518942625 - // - // we use it for controllers, httpContextBase and other request scoped objects: MembershpHelper, TagQuery, UmbracoTreeSearcher and ISearchableTree - // - so that they are automatically disposed at the end of the scope (ie request) - // - not sure they should not be simply 'scoped'? - /// /// One unique instance per request. /// @@ -28,7 +23,9 @@ /// /// Corresponds to /// - /// PerRequestLifeTime in LightInject - means transient but disposed at the end of the current web request. + /// PerScopeLifetime in LightInject. + /// Although it would seem that this should map to PerRequestLifeTime in LightInject, + /// that is actually misleading since PerRequestLifeTime in LightInject means transient but disposed at the end of the current web request. /// see: https://github.com/seesharper/LightInject/issues/494#issuecomment-518493262 /// /// diff --git a/src/Umbraco.Core/Composing/LightInject/LightInjectContainer.cs b/src/Umbraco.Core/Composing/LightInject/LightInjectContainer.cs index d8a554ee8c..35489a06f4 100644 --- a/src/Umbraco.Core/Composing/LightInject/LightInjectContainer.cs +++ b/src/Umbraco.Core/Composing/LightInject/LightInjectContainer.cs @@ -145,7 +145,7 @@ namespace Umbraco.Core.Composing.LightInject /// public void Register(Type serviceType, Lifetime lifetime = Lifetime.Transient) - => Container.Register(serviceType, GetLifetime(lifetime)); + => Container.Register(serviceType, GetLifetime(lifetime, serviceType)); /// public void Register(Type serviceType, Type implementingType, Lifetime lifetime = Lifetime.Transient) @@ -158,7 +158,7 @@ namespace Umbraco.Core.Composing.LightInject case Lifetime.Request: case Lifetime.Scope: case Lifetime.Singleton: - Container.Register(serviceType, implementingType, GetLifetime(lifetime)); + Container.Register(serviceType, implementingType, GetLifetime(lifetime, serviceType)); break; default: throw new NotSupportedException($"Lifetime {lifetime} is not supported."); @@ -169,7 +169,7 @@ namespace Umbraco.Core.Composing.LightInject public void Register(Func factory, Lifetime lifetime = Lifetime.Transient) where TService : class { - Container.Register(f => factory(this), GetLifetime(lifetime)); + Container.Register(f => factory(this), GetLifetime(lifetime, typeof(TService))); } /// @@ -186,7 +186,7 @@ namespace Umbraco.Core.Composing.LightInject where TService : class { // note that there can only be one implementation or instance registered "for" a service - Container.Register(typeof(TService), implementingType, GetTargetedServiceName(), GetLifetime(lifetime)); + Container.Register(typeof(TService), implementingType, GetTargetedServiceName(), GetLifetime(lifetime, typeof(TService))); } /// @@ -194,7 +194,7 @@ namespace Umbraco.Core.Composing.LightInject where TService : class { // note that there can only be one implementation or instance registered "for" a service - Container.Register(f => factory(this), GetTargetedServiceName(), GetLifetime(lifetime)); + Container.Register(f => factory(this), GetTargetedServiceName(), GetLifetime(lifetime, typeof(TService))); } /// @@ -202,14 +202,20 @@ namespace Umbraco.Core.Composing.LightInject where TService : class => Container.RegisterInstance(typeof(TService), instance, GetTargetedServiceName()); - private ILifetime GetLifetime(Lifetime lifetime) + protected virtual ILifetime GetLifetime(Lifetime lifetime, Type type) { switch (lifetime) { case Lifetime.Transient: return null; case Lifetime.Request: - return new PerRequestLifeTime(); + //LightInject behaves slightly differently than all containers and although it would seem that we should be using + //PerRequestLifeTime here, it actually does not mean "one per request", it + //just means transient per request and will be disposed at the end of the request. + //LightInject's PerScopeLifetime is equivalent to "one per request" which is what we'd expect when using the Lifetime.Request since + //that would be consistent with other containers. + //See: https://github.com/umbraco/Umbraco-CMS/issues/6044#issuecomment-518949758 + return new PerScopeLifetime(); case Lifetime.Scope: return new PerScopeLifetime(); case Lifetime.Singleton: diff --git a/src/Umbraco.Core/Umbraco.Core.csproj b/src/Umbraco.Core/Umbraco.Core.csproj index 74af58b94f..a38aba7367 100755 --- a/src/Umbraco.Core/Umbraco.Core.csproj +++ b/src/Umbraco.Core/Umbraco.Core.csproj @@ -71,7 +71,7 @@ runtime; build; native; contentfiles; analyzers all - + diff --git a/src/Umbraco.Tests/Umbraco.Tests.csproj b/src/Umbraco.Tests/Umbraco.Tests.csproj index f788168ddc..cb26aea39b 100644 --- a/src/Umbraco.Tests/Umbraco.Tests.csproj +++ b/src/Umbraco.Tests/Umbraco.Tests.csproj @@ -83,7 +83,7 @@ 1.8.14 - + diff --git a/src/Umbraco.Web/Composing/LightInject/LightInjectContainer.cs b/src/Umbraco.Web/Composing/LightInject/LightInjectContainer.cs index 103cd32912..4717fbd3ff 100644 --- a/src/Umbraco.Web/Composing/LightInject/LightInjectContainer.cs +++ b/src/Umbraco.Web/Composing/LightInject/LightInjectContainer.cs @@ -1,6 +1,10 @@ using System; using System.Web.Http; +using System.Web.Http.Controllers; +using System.Web.Mvc; using LightInject; +using Umbraco.Core; +using Umbraco.Core.Composing; using Umbraco.Core.Composing.LightInject; namespace Umbraco.Web.Composing.LightInject @@ -34,5 +38,32 @@ namespace Umbraco.Web.Composing.LightInject Container.ScopeManagerProvider = smp; // reverts - we will do it last (in WebRuntime) Container.EnableWebApi(GlobalConfiguration.Configuration); } + + protected override ILifetime GetLifetime(Lifetime lifetime, Type type) + { + switch (lifetime) + { + case Lifetime.Transient: + return null; + case Lifetime.Request: + //LightInject behaves slightly differently than all containers and based on feedback from the LightInject authors + //it seems best to use PerRequestLifeTime for controllers even though controllers will work + //just fine with PerScopeLifetime but this is just being 'safe'. + //See: https://github.com/seesharper/LightInject/issues/494#issuecomment-519273614 + //Normally this will return PerScopeLifetime because in LightInject that means "one per request". + //See: base class comments + //See: https://github.com/umbraco/Umbraco-CMS/issues/6044#issuecomment-518949758 + + return type.Inherits() || type.Inherits() + ? (ILifetime)new PerRequestLifeTime() + : new PerScopeLifetime(); + case Lifetime.Scope: + return new PerScopeLifetime(); + case Lifetime.Singleton: + return new PerContainerLifetime(); + default: + throw new NotSupportedException($"Lifetime {lifetime} is not supported."); + } + } } } diff --git a/src/Umbraco.Web/Umbraco.Web.csproj b/src/Umbraco.Web/Umbraco.Web.csproj index 2365017504..02913ac003 100755 --- a/src/Umbraco.Web/Umbraco.Web.csproj +++ b/src/Umbraco.Web/Umbraco.Web.csproj @@ -67,7 +67,7 @@ 2.7.0.100 - +