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.

As part of the migration of components from dotnet/extensions to dotnet/runtime (aspnet/Announcements#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

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 - aspnet/DependencyInjection#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