Make ServiceScope track list of IDisposable as WeakReference

[aspnet-hello]

From @nh43de on Monday, September 11, 2017 12:55:58 PM

Problem: memory leak caused by references to disposed objects in service scope
Summary: We have found that the DI container service scope will hold onto objects that have been disposed, preventing them from being garbage collected.
Location: DependencyInjection/src/DI/ServiceLookup/ServiceProviderEngineScope.cs
Field: _disposables
Root Cause: DI container reference to disposed objects prevents them from being garbage collected
Proposed Solution: replace List to IWeakCollection - collection of weak references. I'm not sure if this is compatible with the design intentions of MS DI.

In ServiceProviderEngineScope.cs, we are tracking a List to track objects that will be disposed when the service scope is disposed. However, this can cause objects that have already been disposed to be retained by the garbage collector, even if the only reference to the object is in the DI container service scope.

In our fork of the project, we have replaced the List with Stephen Cleary's IWeakCollection, and have found that the memory leak goes away. Also, all unit tests pass. This way, the DI container's internal reference to the object does not make the GC consider them as rooted.

http://nitokitchensink.codeplex.com/sourcecontrol/changeset/view/27265#453683

Copied from original issue: aspnet/DependencyInjection#580

[aspnet-hello]

From @khellang on Monday, September 11, 2017 2:24:31 PM

This sounds completely opposite of how scopes are intended to be used. A scope is meant to have a short lifetime (typically a request lifetime). The tracked disposables are owned by the container and are supposed to be diposed by the container.

The solution here would be to register the services as transient and use the container for activation, but take ownership and track instances for disposal yourself. So if you're talking about transient instances, I completely agree with you. In fact, I don't even think transients should be tracked at all (see aspnet/DependencyInjection#456). Unfortunately, the linked issue has been closed, so I wouldn't hold my breath for a fix for these memory leaks in the near future...

[aspnet-hello]

From @nh43de on Monday, September 11, 2017 4:19:38 PM

I was talking about transient services. If we are going to track them, making them weak references allows for GC to collect them when they are no longer referenced and have been disposed.

[aspnet-hello]

From @dotnetjunkie on Friday, September 15, 2017 4:31:14 AM

@nh43de It is the responsibility of the Scope to dispose disposable instances. For it to be able to do this, it requires a (hard) reference to such object. Changing the reference to a weak reference might cause the Scope to lose the reference to it, which makes it impossible for it to call Dispose on such object. This by itself could cause problems such as connection pool timeouts and out of memory problems, because of the undeterministic nature of the GC concerning the invocation of Finalizer methods that need to be called in the absense of a call to Dispose().

[aspnet-hello]

From @nh43de on Friday, October 6, 2017 1:22:06 PM

@dotnetjunkie Thanks for the enlightening comments and I really appreciate you taking the time to respond. This scenario definitely makes sense, and something probably best avoided in favor of proper scope handling.

[AnQueth]

How does this work with something like SignalR that doesn't use HttpContext scoping ? All i have access to is a DefaultDependencyResolver override that can't tell me if its a new request and make a new scope.

[sjb-sjb]

I ran into the same problem. My scope is active for far longer than I want my transient services sticking around. Of course one solution would be to just construct the service object instead of getting a transient one from the service provider. But why solve a problem the easy way when there is a much more interesting way? What I do is define a class Weakenable<T> that allows the user of the transient service to take over responsibility for T.Dispose (see below). Then instead of registering T, just register Weakenable<T>.

public class Weakenable<T>: IDisposable where T: class, new()
{
    Weakenable() => this.StrongReference = new T();

    private T StrongReference { get; set; }
    private WeakReference<T> WeakReference { get; set; }
    public bool IsWeak => (this.StrongReference == null && this.WeakReference != null);

    // When you weaken a transient service, you are taking over responsibility for Dispose.
    // Before the service is weakened, the service provider is responsible. 
    // See https://github.com/aspnet/Home/issues/2336. 
    public T Weaken()
    {
        Debug.Assert((this.StrongReference != null) == (this.WeakReference == null));
        if (this.IsWeak) { throw new InvalidOperationException("Already weak!"); }
        T strongReference = this.StrongReference; 
        this.WeakReference = new WeakReference<T>(strongReference);
        this.StrongReference = null;
        return strongReference;
    }

    public void Dispose()
    {
        if (this.IsWeak) { return; }

        if (this.StrongReference is IDisposable disposable) {
            disposable.Dispose();
        }
    }
}

[aspnet-hello]

We periodically close 'discussion' issues that have not been updated in a long period of time.

We apologize if this causes any inconvenience. We ask that if you are still encountering an issue, please log a new issue with updated information and we will investigate.