transient service is better not be captured by root serviceScope

[yuniansheng]

I am reading the soure code of DI projects, and i found some code in the class CallSiteRuntimeResolver may leading memory issue, below is the method in this class. If we register a service implemented the IDisposable interface with a transient lifetime, and then get this service from the root ServiceProviderEngineScope, the root scope will hold the reference of this service because the VisitDisposeCache method called Scope.CaptureDisposable, CaptureDisposable's inner logic will add this service to it's _disposables list, so this service will never be collected by GC, if the service has some non-managed resource, this resource will never be disposed.

        protected override object VisitDisposeCache(ServiceCallSite transientCallSite, RuntimeResolverContext context)
        {
            return context.Scope.CaptureDisposable(VisitCallSiteMain(transientCallSite, context));
        }

I suggest adding an if condition in the VisitDisposeCache method, if the context.Scope is the Root scope then just return the resolved service else call the CaptureDisposable to add the service to scope's _disposables list

[davidfowl]

This is a known issue with the container and it would need to be opt out at this point. If we don't capture transient disposables in the root scope then they wouldn't get disposed when the container is disposed.

[ghost]

As part of the migration of components from dotnet/extensions to dotnet/runtime (https://github.com/aspnet/Announcements/issues/411) we will be bulk closing some of the older issues. If you are still interested in having this issue addressed, just comment and the issue will be automatically reactivated (even if you aren't the author). When you do that, I'll page the team to come take a look. If you've moved on or workaround the issue and no longer need this change, just ignore this and the issue will be closed in 7 days.

If you know that the issue affects a package that has moved to a different repo, please consider re-opening the issue in that repo. If you're unsure, that's OK, someone from the team can help!

[tim-fernico]

I have been caught by this issue, it needs to be addressed

[ghost]

Paging @dotnet/extensions-migration ! This issue has been revived from staleness. Please take a look and route to the appropriate repository.

[ericstj]

@davidfowl what about WeakReference? Would that solve this compatibly?

[maryamariyan]

@davidfowl should this be considered for 6.0?

[davidfowl]

@davidfowl what about WeakReference? Would that solve this compatibly?

Dispose needs to be explicitly called and WeakReference would add overhead.

@davidfowl should this be considered for 6.0?

We could add a global option but nothing per service. That requires other containers to buy in.

[davidfowl]

History - https://github.com/aspnet/DependencyInjection/issues/456

I say, lets add a flag for this and have it apply to the root scope only:

API proposal

namespace Microsoft.Extensions.DependencyInjection
{
    public class ServiceProviderOptions
    {
+       public TransientBehavior TransientBehavior { get; set; }
    }

+    public enum TransientBehavior
+    {
+        Default,
+        WeakDisposableCaptureInAllScopes,
+    }
}

The name is verbose but explicit. I think we have 2 options here:

• Don't track them at all - It means the user is responsible for tracking the transitive closure of disposable objects and disposing them.
• Track them with WeakReferences as @ericstj mentions and make it a best effort for disposal

[dazinator]

It may be better to make this option an enum, with the options to:

• preserve legacy behaviour (I.e no change)
• disable in root scope only (as you've proposed)
• disable for all scopes

Update: looks like api proposal does have this I must have missed it on first glance!

[davidfowl]

Disabling in root scope only seems oddly asymmetric and that could be problematic in my view. For example if a class is injected with a transient disposable dependency - if that class is resolved in root scope it would be responsible for disposing of its dependency, but if its resolved in non root scope it wouldn't be. Yet it has no control over what scope/s it's potentially resolved in so how does it know what to do?

This is a problem regardless. Usually the actor that controls disposing the container doesn't control all objects that go into the container. How would a library every know if this switch is on or not since they can no longer predict if they need to call dispose or not? The other complication comes from the fact that we capture the disposable based on the implementation type, not the service type. That means that it's possible to set this flag and leak dependencies you don't own because they were relying on somebody else calling dispose. The reason I like the root container only option is because I think that's the only problematic case, other scopes get disposed more often than once per application.

Though if you were doing something where you had long running application scoped tenants (like I assume you do) I can see why you'd run into the same issue. Though if you were creating multiple root container instances you could configure it there.

What you say tough makes me remember some of the initial discussion around this where objects need to be designed around the fact that they hold onto disposable objects. The problem is that today the DI container lets you skip thinking about this because its the container's job. As a result you can have objects graphs like this:

A ->B (disposable) -> C-> D (disposable)

The DI container sits between this object graph and will dispose B and D when the scope is disposed. Manually disposing this object graph, you either need the entire graph to be disposable so that disposing A disposes everything else, or I need to somehow track disposable objects myself.

Maybe this is a bad idea and we should have a single option to use a weak reference instead of a strong reference is better for all scopes, not just the root scope.

Updated the API to reflect this. Let me know your thoughts.

[dazinator]

@davidfowl Yes I'm having very similar thought patterns.

Root scope is only problematic in this case because its typically long living. However in unit test scenarios creating a root SP per unit test and disposing of it at the end of each test- there is no issue.

With that in mind I'm thinking this issue could be addressed by adding a single opt in behaviour, to throw when a non singleton IDisposable is resolved (injected) in "root" scope.

Hostbuilder scenarios (typically Hostbuilder means an actual application scenario) could potentially opt in to this by default (bit risky) as its expected the root SP that it builds will be a long living one for a long running host. However unit test code or code that directly creates a new ServiceProvider - could remain an explicit opt in. Just an idea.

This opt in behaviour would add a small safety net of sorts. If the exception is thrown its because you are most likely resolving transient or scoped IDisposables from root scope which is most likely not want you want to be doing. The answer isn't to disable tracking though (because we have no other agreed way to control disposable ownership with a mechanism that all containers conform too yet) but its instead to make sure you create a scope and resolve from that, then dispose the scop - the exception would indicate this.

This does special case the root scope a bit though. In theory any scope could be made a long living scope and exhibit this problem. Therefore it might be an idea to also expose the same behavioural options when creating a new scope. If you are creating a new scope for a very long running background job you might want to opt in to this same option for all of the same reasons.

[springy76]

Wow 7+ years of discussion, nothing gained. Can I control this in a better way using AutoFac, DryIOC (or whatever is hip in 2023)? (I'm using Blazor-Server, so there exist long living service scopes.)

Btw, there is one disposable transient service in the framework: Microsoft.AspNetCore.Routing.Matching.DataSourceDependentMatcher+Lifetime - can't tell if it is a candidate for memory leaks.

[dazinator]

I still think adding the following would solve this issue:

• Add an opt in, to throw a runtime exception if an IDisposable service registered as Transient or Scoped is resolved in root scope. (Singleton is fine).
• Add an opt in, for .CreateScope() - to throw a runtime exception if an IDisposable service registered as Transient is resolved in that scope. Assume scoped IDisposable's resolved in a scope are ok due to the concept that scopes should have fixed lifetimes shorter than the application lifetime.

[steveharter]

@davidfowl any thoughts on the above proposal?

[OvidiuTK]

One workaround with factory delegate which would allow decoupling: services.AddTransient<Func<IFooDisposable>>(sp => () => new FooDisposable()); and the consumer: public class Consumer { Func<IFooDisposable> _factoryDelegate; public Consumer2(Func<IFooDisposable> factoryDelegate) { _factoryDelegate = factoryDelegate; } public void DoSomeWork() { // We can repeat this as many times as we want to, because fooDisposable will be disposed when we want to. using (IFooDisposable fooDisposable = _factoryDelegate()) { ... } } } Where: public interface IFooDisposable : IDisposable { /*...*/ } So I would lean toward a compile warning with a message hinting to the use of a factory delegate instead. Also an opt-in or opt-out "object not disposed" compile warning in general may help, when a path not disposing a disposable object exists. Based on #36491 comment by @dazinator

[steveharter]

Moving this for v9

[ChiefInnovator]

The reason I like the root container only option is because I think that's the only problematic case, other scopes get disposed more often than once per application.

I think this needs to be with all containers. There are scopes such as Login to Logout in a Desktop application which has a long lifetime, almost the entire lifetime of an application. Yet there are tons of Transients in that scope that you want Disposed independent of Container lifetime.