Nested lifetime scopes aren't disposed when the parent is disposed

[alexmg]

From travis.illig on December 08, 2012 04:00:45

var builder = new ContainerBuilder();
var container = builder.Build();
var 11 = container.BeginLifetimeScope();
var l2 = l1.BeginLifetimeScope();
var l3 = l2.BeginLifetimeScope();
l1.Dispose();

The nested lifetime scopes l2 and l3 should also be disposed but they aren't. That could cause some oddness if the nested scopes are using any singletons or objects that are members of parent scopes - the dependencies up the chain will be disposed out from underneath the child scope objects.

See forum post here: https://groups.google.com/forum/?fromgroups=#!topic/autofac/So_dlw_13po

Original issue: http://code.google.com/p/autofac/issues/detail?id=397

[alexmg]

From travis.illig on December 14, 2012 12:04:44

Status: Started
Owner: travis.illig

[alexmg]

From travis.illig on December 14, 2012 12:30:52

This issue was closed by revision d656503e705e .

Status: Fixed

[alexmg]

From INick...@gmail.com on December 16, 2012 06:39:51

Super! I wait for this feature about 3 years.

[alexmg]

From INick...@gmail.com on December 16, 2012 06:41:07

I hope this bug-fix dont't break my existing code...

[alexmg]

From travis.illig on February 07, 2013 10:32:03

I'm re-opening this issue because I'm rolling the feature back out.

We found that there is a memory leak with the initial approach. We were holding onto lifetime scopes to ensure they'd get disposed... but that meant we had strong references to the child scopes and garbage collection couldn't pick them up even if they were manually disposed. Not good.

When we end up addressing the issue, we need to be mindful of how we track child scopes. For example, we may want to use a list of WeakReferences so garbage collection can take place and dispose scopes during finalization but any that aren't handled that way could still be disposed by the parent.

Something like this in LifetimeScope...

// Collection of child scopes
private List _childScopes = new List();

// Remove dead references, add a weak reference to the new
// child scope, and trim any list excess so the list itself
// doesn't balloon.
private void TrackChildScope(ILifetimeScope scope)
{
this._childScopes.RemoveAll(w => w.Target == null);
this._childScopes.Add(new WeakReference(scope));
this._childScopes.TrimExcess();
}

// Then when you create a new child scope...
this.TrackChildScope(scope);

// ...and in the dispose implementation, after the Disposer runs...
foreach(var weakRef in this._childScopes)
{
// Get a reference so it's not cleaned up from underneath you
var scope = weakRef.Target;
if(scope != null)
{
((ILifetimeScope)scope).Dispose();
}
}

I think something like that looks like it'd be a bit safer and would let garbage collection happen while still allowing live child scopes to be disposed with the parent. However, some mechanism of testing would have to occur to verify that - maybe attach a profiler to a test app that generates tons of scopes, abandons them, and then forces garbage collection? We'd want to test this pretty hard and verify there's not something being overlooked.

Sorry to folks who wanted this just to have it yanked out from under them... and sorry to folks who got hit by the memory leak.

Status: Accepted
Owner: ---
Labels: -Priority-High Priority-Medium

[alexmg]

From travis.illig on February 20, 2013 07:57:56

We talked about it a lot in the forum thread and offline in various email chains. There are a lot of pros and cons to implementing auto-disposal of nested scopes. Obviously it's a bad place to be if you have a child scope you're resolving from but the parent has been disposed and it'd be nice to avoid situations like that.

However, as we discovered, adding auto-disposal comes with no small amount of complexity and maintenance cost. Even if we were to get into WeakReferences and tight memory management, we have to weigh the cost/benefit on that.

In the end, we decided not to implement the feature.

If you're carefully managing your scopes (or using one of the provided integration libraries) it's a rare time when you'll have an outer scope get disposed but an inner scope still alive. The majority use case (not counting the integration libraries) is to wrap scope creation in a "using" even when nesting, which will do the cleanup for you. If you're not wrapping with "using," then you're already into some advanced territory and we have to trust that you'll manage your resources appropriately.

Status: WontFix
Owner: travis.illig
Cc: alex.meyergleaves

[alexmg]

From nikitin....@gmail.com on March 16, 2013 09:58:11

What if we will track a child scope from the parent scope. And remove a reference from the parent while disposing the child. This should cause no memory leaks.

Here is my view in patch file.

Attachment: Issue397.patch

[alexmg]

From nikitin....@gmail.com on March 19, 2013 11:51:41

Pushed the code here: http://code.google.com/r/nikitinalexandra-autofac/source/detail?r=b62480e7345699e3bd8428857de0a5bfd7693208

[alexmg]

From travis.illig on March 19, 2013 11:58:49

Thanks for the push.

The problem here is that if the person doesn't actively dispose the scope, the Disposer will still hold a strong reference to it. Garbage collection won't ever pick up the LifetimeScope for disposal because there is a reference to it - in the Disposer. Memory leak.

Of course, if the person is actively disposing their scopes properly, the whole issue goes away.

You get into holding WeakReferences to child scopes, only disposing when the WeakReference is still alive, etc. Even if you track WeakReferences, you have the potential memory leak where the list of WeakReferences just increases ad infinitum but is holding references that have been cleaned up, so you have to trim the list of WeakReferences periodically.

You can see some of this in the sample code I mentioned above. This sort of complexity is why we decided not to implement the fix.

Again, thanks, but I don't think we'll be acting on this request at this time.

[alexmg]

From nikitin....@gmail.com on March 20, 2013 05:07:31

Travis,
Thanks for your answer and sorry for persistence :))

I do not understand one thing:
Why do not we treat a child lifetime scope as any disposable object in the lifetime scope? Why should the behavior be different. I mean WeakReferences.

We do not store WeakReferences for a disposable object within the lifetime scope. The lifetime scope holds a reference to the diposable object. And it's also a cause for memory leaks.

Why do not we use the same behavior?
And how does child lifetime scope differ from a disposable object?

Thank you.

[alexmg]

From travis.illig on March 20, 2013 10:52:47

Lifetime scopes hold onto disposable object references for both disposal and sharing purposes. If you have "instance per lifetime scope," instances need to hang around so they can be shared. Same with singleton instances. At the end of the lifetime scope, appropriate items get disposed. If we used WeakReferences on items INSIDE a lifetime scope, then sharing would break - things would get cleaned up out from under us by Garbage Collection and you couldn't have "instance per lifetime scope" anymore.

It's probably best to not think of a lifetime scope as an "object." It's more a... controller. It controls the lifetime of other objects. You can sort of think of a lifetime scope like a big "using" statement that wraps everything.

The trouble starts if you have lifetime scopes with long lives. If a scope lasts a long time, you will start seeing what appears to be a memory leak even if you are just resolving instance-per-dependency items that are disposable - because they'll be held onto until the lifetime scope disappears.

This becomes really obvious when you consider that the root application container is a lifetime scope so if you're resolving stuff out of the root container, every disposable object is being tracked until that container is disposed.

So: Lifetime scopes being disposable objects, let's say we do track every lifetime scope not using weak references. Web server fires up, creates the root container, and 100,000 users hit that thing. That's 100,000 request lifetime scopes, all of which are tracked now by the root container.

If we WAIT to dispose the request lifetime scopes until the parent scope is disposed, we have the problem that I introduced - huge memory leak.

Why don't you see a leak today? Because lifetime scopes are short-lived and there's a mechanism in place for automatically disposing of them at the end of a request.

If we track and dereference scopes as they are manually disposed, that's a little better, but it still adds the overhead of tracking... and, again, if folks are diligent in disposing, we don't have the requirement to track anyway.

The real question is whether the feature is valuable enough to justify the additional complexity. There are a lot of use cases to consider. For example:

User creates a lifetime scope A from a container. User creates lifetime scope B from lifetime scope A. User holds a reference to B but lets A go out of scope. Does B get disposed when garbage collection runs?

If we do the "scope removes itself from the parent on disposal" solution, without WeakReference, A doesn't actually EVER go out of scope because B would hold a reference to its parent. Memory leak.

If we use WeakReferences, we introduce the need to clean up the list of child scopes periodically as seen in my code snippet above, otherwise... Memory leak.

Given the complexity and the fact that folks creating lifetime scopes really just need to be detail oriented, we decided not to implement the feature. If you're using the existing integration, this shouldn't bother you. If you're writing your own integration, you already need to be paying attention - just like when you're writing multi-threaded code or anything else with a level of complexity.

If you'd like to continue the discussion or still have additional questions, let's take it over to the discussion forum rather than using the issues list as a forum. That will also allow other folks to chime in with their thoughts. Thanks! https://groups.google.com/forum/?fromgroups=#!topic/autofac/So_dlw_13po