DI Lifetime.Request works per thread.

[ekovandbor]

Brief explanation of issue on forum: https://our.umbraco.com/forum/umbraco-8//98389-iusercomposer-with-lifetimerequest-works-as-lifetimetransient

In general: All would have been ok with DI, if i didnt need custom EF db context, not related to umbraco content - my own db for a lot of businesslogic data structure, for webapi and custom mvc.

So issue in general: EntityContext.Instance() gets fired 35 times during request. I did static counter, and it is seen that it was created 35 times during request. Why Lifetime.Request doesnt create 1 instance per request? (all server side use async awaited Tasks)

Apart from that Lifetime.Request dependencies gets disposed at the end of first thread, so in controller constructor i have ok context, but in controller method it is already disposed. Also every single dependency on EntityContext has its own instance, instead 1 instance per request. So my db context and transactions are disposed before being injected. And all dependencies have different db contexts - checked by static counter with instance saved value.

Entire concept of UnitOfWork with services breaks, as i cant handle all db updates at once which is important for business logic.

I tried to create own Container and resolve DI with DependencyResolver, but entire backoffice stop to work - i can't even login (so umbraco uses DependencyResolver), also umbraco dependent instances wasnt loaded.

Is it a known issue? i had 8.0.2 version, and then updated to 8.1 - issue still exist. The only specific thing is that i extended UmbracoDefaultOwinStartup, as it was needed for webapi. rest shouldn't do anything to default installation.

public partial class Startup : UmbracoDefaultOwinStartup
    {
        public override void Configuration(IAppBuilder app)
        {
            base.Configuration(app);

            ServicesConfig.RegisterServices(app);          
            FluentValidationModelValidatorProvider.Configure(x => x.AddImplicitRequiredValidator = true);
            AuthConfiguration.ConfigureAuth(app);
        }

        protected override void ConfigureServices(IAppBuilder app, ServiceContext services)
        {
            base.ConfigureServices(app, services);
            ServicesConfig.RegisterServices(app, services);            
        }

        protected override void ConfigureMiddleware(IAppBuilder app)
        {
            base.ConfigureMiddleware(app);            
        }        
    }

Would be cool to get some answer or suggestions on forum :)

P.S. 1 little update - just have tried after post. changed back owin startup to UmbracoDefaultOwinStartup. And now all instances being disposed after controllers method execution. Tada! (so extending UmbracoDefaultOwinStartup causes DI issues?) but still instead of 1 instance per request - many instances.

[kjac]

@ekovandbor I believe you need to use PerScopeLifetime for this. See http://www.lightinject.net/web/ for documentation.

Also... as this is not a bug nor a feature request, please consider keeping these types of questions in the forum.

[nul800sebastiaan]

Yep, questions like these are better for the forums at https://our.umbraco.com

I'll close this one unless we can identify and actual issue. 👍

[ekovandbor]

@kjac, Can confirm that per scope lifetime works when container configured for web. Thx! To @umbraco team: please update your documentation then. You forcing to use Lifetime.Request which doesnt work at all as expected, registering services with lifetime Request - works unpredictable, creating many instances instead 1, also disposes objects in the middle of request. Lifetime.Scope works perfectly for web.

[nul800sebastiaan]

Could be good to point out where our documentation is wrong @ekovandbor :-) Got a link?

[ekovandbor]

I mean, you provide Lifetime.Request behaviour for Register component, which doesnt work for web request. Lifetime.Scope has to be used instead together with preparing container for using mvc, webapi, perwebrequestscope. And only this works. Lifetime.Request seems to work as transient, no metter that container configured for web.

[nul800sebastiaan]

So.. we're at least missing some documentation then?

FYI: I know very little to nothing about the DI implementation, so I may be asking silly questions. :)

To make the life of an Umbraco dev better: should we document this, should we change a default (which we probably can't sounds like it would be a breaking change), or..?

[ekovandbor]

Idk, i just leave it here up to you. As for me having Lifetime.Request has to work per request or be renamed to idk what it was developed for :) Have a documentation for such problem place coud have helped. I think you know v8 lacks documention so hard.

[kjac]

@ekovandbor as you point out the V8 docs are lacking a bit, and the IoC docs are no exception; https://our.umbraco.com/Documentation/Reference/using-ioc is still the V7 version.

However.

The lifetimes provided by Umbraco currently mimics those provided by LightInject, which is the default DI for Umbraco. That's why Request doesn't work as you'd expect. You can read more about the lifetimes here and specifically web oriented lifetimes here.

@nul800sebastiaan I believe there are two things to look into here:

1. Write some V8 docs for the IoC part - and as a bare minimum make sure that the current ones list as "not yet V8-ified".
2. Have someone at the core team review this comment. It may be plausible to change the default behavior of Request to actually be web request scoped (essentially make Request use Scope or something along those lines). Not however that this would be a major breaking change for all V8 implementations with custom IoC setups, not to mention any additional IoC container providers being built.

[Shazwazza]

I don't know the history of this comment https://github.com/umbraco/Umbraco-CMS/blob/853087a75044b814df458457dc9a1f778cc89749/src/Umbraco.Core/Composing/Lifetime.cs#L18

However it is misleading since we don't use that for UmbracoContext, we manage it's lifetime manually. IMO Lifetime.Request should mean "per-request" and that is the case for LightInject and it does equate to PerRequestLifetime, the code is here: https://github.com/umbraco/Umbraco-CMS/blob/v8/dev/src/Umbraco.Core/Composing/LightInject/LightInjectContainer.cs#L212

But perhaps the reason for the comment is that some containers behave differently in some scenarios including LightInject, i just saw this comment: https://github.com/seesharper/LightInject/issues/131#issuecomment-63486075. The documentation for LightInject lifetimes is here https://github.com/seesharper/LightInject#lifetime and there's a detailed section about async/await: https://github.com/seesharper/LightInject#async-and-await

In that section it goes onto say that the ScopeManagerProvider would need to use PerLogicalCallContextScopeManagerProvider. However, in Umbraco it is using a custom provider: https://github.com/umbraco/Umbraco-CMS/blob/v8/dev/src/Umbraco.Core/Composing/LightInject/MixedLightInjectScopeManagerProvider.cs ... which essentially boils down to using PerWebRequestScopeManagerProvider as per the LightInject docs.

We use this Lifetime.Request internally for all controllers, httpcontext, membershiphelper, tagquery, umbracotreesearcher, and these all seemingly behave as intended. However, I've just tested using async/await in a controller like this:

public async Task<ActionResult> Default()
{
    var m1 = this.Members; //this is injected into the ctor
    var m2 = await GetMembershipHelper(); //This creates another instance!! :/
    //... 
}
private async Task<MembershipHelper> GetMembershipHelper()
{
    await Task.Delay(1000);
    return await Task.FromResult(Current.Factory.GetInstance<MembershipHelper>());
}

And even though MembershipHelper is declared as Lifetime.Request which uses LightInject's PerRequestLifetime, it doesn't work as assumed and another instance is created!

Part of the solution must revolve around using PerLogicalCallContextScopeManagerProvider but I've tried updating the code to use PerLogicalCallContextScopeManagerProvider and PerScopeLifetime instead of PerRequestLifetime (as per LightInject docs) and I get other errors (will report to Light Inject).

So I think we need to understand what is going on and how things are supposed to be working. AFAIK the httpcontext should flow with async/await which is probably why in theory PerWebRequestScopeManagerProvider should work since the scope is tied to the HttpContext but for some reason it's not ...

[Shazwazza]

Quick update on this and it's a little confusing, in the docs for LightInject with MVC it actually states that controllers will use PerRequestLifeTime but that "services" that need to be disposed of at the end of a request should be PerScopeLifetime... which is pretty confusing.

I have raised a question on the Light Inject GH tracker https://github.com/seesharper/LightInject/issues/494

I have a simple local test that shows if you want services to be 'per-request' you need to use PerScopeLifetime - which would equate to Lifetime.Scope. So I 'think' what this comes down to with LightInject is:

• PerRequestLifeTime is purely used to dispose objects created during a request at the end of the request, this doesn't mean they'll only be created once per request
• PerScopeLifetime will only create one object per scope and dispose at the end of that scope and within the context of a request, the request is the scope.

This will also mean that there's some services registered in Umbraco that are incorrect and would need to be updated. But then I wonder if there's really any reason to use PerRequestLifeTime at all - I've also tested registering controllers with PerScopeLifetime instead of PerRequestLifeTime and that works as expected too.

[kjac]

Awesome investigation there @Shazwazza 😀

Personally I think "Scope" is a counter intuitive name for a lifetime. If it was removed from the Umbraco lifetime definition and then internally Request was mapped to Scope, people probably wouldn't miss Scope a whole lot?

It would probably also make it simpler to create IoC implementations for other containers.

But of course... It's a breaking change with a somewhat capital B.

[Shazwazza]

@kjac we'll see what Light Inject say, i feel like it's a bug that PerRequestLifeTime creates multiple instances in the same request. Like i mention in that other thread, the docs here https://www.lightinject.net/#perrequestlifetime say

A new instance is created for each request

Which seems like it would indicate "once per request"

Otherwise we'll consider our options :)

[ekovandbor]

@Shazwazza, @kjac . Sorry guys for tagging, but i dont expect anybody else to reply here, and need some quick suggestion, if it ever could be found.

I thought i resolved DI for asp pages for ucommerce backoffice extensions, but issue is still there.

DI works. All instances are created once per request and disposed after, just with very strange issue for all requests which go after first request. During first request (where pipeline loads usercontrol - which injects all dependencies) - everything works fine - get/add/update operations work. Second and all next requests (pipeline executes anew for every single click in backoffice)- injected are old disposed instances, but new instances are created too at the same time, before actual injection. So with a static counter i can see that created was instance N2, but injected was N1.

So new EntityContext was created, but injected is old, where context is disposed. Dispose method is not fired during that process - just at the end of request. So new context gets disposed after actual EntityContext (N1) in repository throws exception so request ends: The operation cannot be completed because the DbContext has been disposed. Error occurs in a line which creates DbSet for generic repository: Context.Set<T>()

So question. Why old instances, which are disposed, aren't deleted after, and are used for resolving dependencies, when new instances are created at the same time for a reason to be injected? Why old aren't deleted from resolver?

Usercontrol injection looks like this:

public partial class AddTripsSection : UserControl
    {
        private ITripsService _tripsService { get; set; }
        private IUnitOfWork _unitOfWork { get; set; }

        public AddTripsSection() : this(Current.Factory.GetInstance<ITripsService>(), Current.Factory.GetInstance<IUnitOfWork>())
        {
        }

        public AddTripsSection(ITripsService tripsService, IUnitOfWork unitOfWork)
        {
            _tripsService = tripsService;
            _unitOfWork = unitOfWork;
        }
}

Used also DependencyResolver.Current.GetService() - similar result. Also moved injections to exact method, which needs dependencies and executes logic. Same result.

So, for async .aspx and .ascx for first request all dependencies are ok, and all db operations work. For all next requests - where page lyfecycle executed anew, and all depencies injected anew - we receive disposed instances which weren't removed from resolver. For mvc and webapi, even after this moment all works just perfectly.

Any suggestions? This stops work at extending backoffice, as DI works just for the first request after project execution, all next requests get old disposed instances. All this is relevant for a scoped objects inside PerRequestScope, transient works well.

Isn't this a way for a memory leak inside umbraco composing?

[nul800sebastiaan]

Just as a little hint here: none of what you're trying to do in v8 is known to anyone at the moment, so I wouldn't expect too much help here. While I'll readily admit I am not a good enough developer to understand exactly what you're trying to do, it really feels like you're mixing MVC and UserControls and I can't imagine that's supported very well.

I would recommend either digging into the source code of whatever is not working for you, or maybe ask uCommerce for best-practice advise here.

[ekovandbor]

MVC or ASP, uCommerce or not, this is DI issue. All renderings go through request, so RequestContext is the same, and scope should have worked. And it works, but properly - for a first request only. The only issue here is that old disposed instances aren't deleted afterwards and are being used for next injections in new requests. Idk if this is related to umbraco composing or LightInject, but probably a field for investigation for your team. It will lead to a memory leak, or at least to unproper work for your customers. And as you now provide a core for injections, and we stock to that - i can't define own resolver for DependencyResolver.Current as all backoffice fails - i suppose umbraco need to take care of such issues

Autofac worked well in v7 for usercontrols.

[nul800sebastiaan]

Yep, it might well be an issue in Umbraco. We'll investigate in due time. Please understand that this is not a high priority for us right now and the best way to get help at the moment is to help yourself (dive into the source code to see if you can spot a problem).

[Shazwazza]

@kjac They responded, see here https://github.com/seesharper/LightInject/issues/494#issuecomment-518581843 And re-reading this comment https://github.com/umbraco/Umbraco-CMS/blob/853087a75044b814df458457dc9a1f778cc89749/src/Umbraco.Core/Composing/Lifetime.cs#L18 indicates exactly that. This TODO in the code previously was a fixme and should have been reviewed prior to release but unfornately that didn't happen and now we need to make some decisions about what to do. As for the term Scope regarding lifetimes in DI, it's a common term among all containers so not something we can just rename.

I've pushed some code comments to this rev: 467cf8d76235fb421876db66086a33fd7c3cf4e6

Based on all other containers, LI is the only one that doesn't behave in the same fashion for Lifetime.Request as you can see in the updated comments in that rev. I've asked another question to LI here https://github.com/seesharper/LightInject/issues/494#issuecomment-518942625 because in my tests if we 'just' map Lifetime.Request to LI's PerScopeLifetime then functionality will match with all other containers and we will get the results that we expect.

I don't believe that this will be a breaking change to do since we've all expected that Lifetime.Request would be "one per request", not transient per request with per request disposing.

@ekovandbor Are you following along with what we are saying? If so, can you just change your registrations from Lifetime.Request to Lifetime.Scope and see if all of your problems are fixed. Your issue sounds complex and not something that i understand since i'm not working with your codebase. If you still have issues using Lifetime.Scope then you'll need to create a vanilla Umbraco solution with no plugins and the absolute bare minimal amount of code to replicate the problem and then share that.

[kjac]

@Shazwazza I think you're correct. Mapping Request to PerScopeLifetime internally was kinda what I was rambling about earlier 😄 I guess my other point was that maybe Scope should be removed entirely from the lifetimes supported by the Umbraco IoC abstraction layer. From a functionality perspective this is effectively what said mapping would do anyway, so I figured it wouldn't make sense to keep two differently named Umbraco lifetimes with the same default (core) behavior.

As for it being a breaking change or not, well... as long as Scope remains and stays mapped to PerScopeLifetime as well, the change remains a non-breaking bugfix.

[Shazwazza]

@kjac Yep I agree. What we can do is obsolete and hide Lifetime.Scope since internally we don't use it (well we actually do, but that's because we actually wanted 'one per request' in some cases). In some cases people would want to use the container's own Scope based lifetime but in those cases, developers can do this directly using the container by accessing the IRegister.Concrete property to return the concrete underlying container. I can submit a PR shortly for 8.2.

In the meantime, anyone wanting to have a lifetime of "one per request" must use Lifetime.Scope

[ekovandbor]

@Shazwazza, reading every comment :) Used directly LI container with PerScopeLifetime. Can confirm that Lifetime.Scope even without composition's .ConfigureForWeb() method does exactly what is expected from PerRequest instantiation. The only issue which left is listed above with using DI in ucommerce backoffice. I wonder why i keep getting old disposed instances injected.

[Shazwazza]

Have created a PR here https://github.com/umbraco/Umbraco-CMS/pull/6069

Have decided to not obsolete Lifetime.Scope since there will be use cases for this for some people and since this is a native feature for every container.

[umbrabot]

Hiya @ekovandbor,

Just wanted to let you know that we noticed that this issue got a bit stale and might not be relevant any more.

We will close this issue for now but we're happy to open it up again if you think it's still relevant (for example: it's a feature request that's not yet implemented, or it's a bug that's not yet been fixed).

To open it this issue up again, you can write @umbrabot still relevant in a new comment as the first line. It would be super helpful for us if on the next line you could let us know why you think it's still relevant.

For example:

@umbrabot still relevant This bug can still be reproduced in version x.y.z

This will reopen the issue in the next few hours.

Thanks, from your friendly Umbraco GitHub bot :robot: :slightly_smiling_face: