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

[GoogleCodeExporter]

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 reported on code.google.com by travis.illig on 7 Dec 2012 at 6:00

[GoogleCodeExporter]

Original comment by travis.illig on 14 Dec 2012 at 8:04

• Changed state: Started

[GoogleCodeExporter]

This issue was closed by revision d656503e705e.

Original comment by travis.illig on 14 Dec 2012 at 8:30

• Changed state: Fixed

[GoogleCodeExporter]

Super! I wait for this feature about 3 years.

Original comment by INick...@gmail.com on 16 Dec 2012 at 2:39

[GoogleCodeExporter]

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

Original comment by INick...@gmail.com on 16 Dec 2012 at 2:41

[GoogleCodeExporter]

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<WeakReference> _childScopes = new List<WeakReference>();

// 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.

Original comment by travis.illig on 7 Feb 2013 at 6:32

• Changed state: Accepted
• Added labels: Priority-Medium
• Removed labels: Priority-High

[GoogleCodeExporter]

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.

Original comment by travis.illig on 20 Feb 2013 at 3:57

• Changed state: WontFix

[GoogleCodeExporter]

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.

Original comment by nikitin....@gmail.com on 16 Mar 2013 at 4:58

Attachments:

• Issue397.patch

[GoogleCodeExporter]

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

Original comment by nikitin....@gmail.com on 19 Mar 2013 at 6:51

[GoogleCodeExporter]

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.

Original comment by travis.illig on 19 Mar 2013 at 6:58

[GoogleCodeExporter]

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.

Original comment by nikitin....@gmail.com on 20 Mar 2013 at 12:07

[GoogleCodeExporter]

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

Original comment by travis.illig on 20 Mar 2013 at 5:52