trquth / autofac

Automatically exported from code.google.com/p/autofac
Other
0 stars 0 forks source link

Default behavior with disposables makes it easy to create memory leaks #481

Closed GoogleCodeExporter closed 8 years ago

GoogleCodeExporter commented 8 years ago
What steps will reproduce the problem?
1. register some IDisposable service as InstancePerDependency()
2. resolve instances of that service from a static (root) container
This happened to us when using Autofac's WebApi integration. We had the 
following setup:

// when configuring autofac
containerBuilder.Register<MyDisposableService>().InstancePerDependency();
containerBuilder.Register<AuthHelper>().SingleInstance();

// in the authorization attribute
// because we're in the attribute, this resolution happens in the root
// scope rather than the request scope
actionContext.ControllerContext.Configuration.DependencyResolver
    .GetService(typeof(AuthHelper));

// in AuthHelper
public AuthHelper(Func<MyDisposableService> factory) { ... }

public void Help() {
    using (var service = this.factory()) { /* do stuff with service */ }
}

Basically, because the attribute resolves from the root, all of the 
MyDisposableService instances created by the AuthHelper would be tracked by the 
root scope's disposer forever, thus creating the leak.

When we finally diagnosed the problem, the fix was to have AuthHelper depend on 
a Func<Owned<MyDisposableService>> instead.

What is the expected output? What do you see instead?
In general, Autofac's treatment of IDisposables is extremely helpful, however, 
I think some validation could be added to help people avoid this.

For example, a ContainerBuilderOption could be added called "StaticContainer" 
to make autofac aware that this scope will be long-lived. Thus, you'd have:

var container = builder.Build(ContainerBuilderOptions.StaticContainer);
// because of the option, this would throw an exception with the following
// message:
// "autofac does not automatically disposing IDisposable services when
// they are resolved from a static root container. Register the service as
// SingleInstance() or Resolve an Owned<TService> instance to manage
// disposal manually
container.Resolve<SomeDisposableRegisteredAsAnythingButSingleInstance>();

I suppose another approach would be to have the Disposer hold weak references 
to the IDisposables (therefore assuming that they have destructors as 
necessary). Periodically (e. g. when adding a new disposable), dead references 
could be cleaned up

What version of Autofac are you using? On what version of .NET/Silverlight?
3.0.2, .NET 4.5

Please provide any additional information below.

Original issue reported on code.google.com by mike.ade...@gmail.com on 3 Jan 2014 at 10:56

GoogleCodeExporter commented 8 years ago
The issue you describe is why we have documented on the best practices wiki 
page that you should always resolve dependencies from nested lifetime scopes:
https://code.google.com/p/autofac/wiki/BestPractices#Always_Resolve_Dependencies
_from_Nested_Lifetimes

One way you can fix this for your components that must be IDisposable but 
resolved from the root container is to mark them ExternallyOwned. That's on the 
wiki here:
https://code.google.com/p/autofac/wiki/DeterministicDisposal

Alternatively, as you found, you can use the Owned<T> relationship to 
automatically disable the tracking.

Using DI and properly configuring it is a somewhat advanced thing that is 
really hard to help people "stop shooting themselves in the foot." We can't 
anticipate whether you'll be resolving an IDisposable component from the root 
of the container. We don't know if you're building a temporary container and 
it's OK to allow you to resolve references to singleton IDisposables right out 
of the container. There are numerous other situations where you can also get 
into a sticky place if you're not careful. (Create a nested lifetime scope and 
forget to clean it up?)

Unfortunately what that all amounts to is that there's no great way to add 
"sanity checking" like this. We have to trust that the application developer is 
doing proper memory management, lifetime scope management, and so forth.

I'm glad you caught the issue in your app, but I don't think we'll be 
implementing safeguards around this.

Original comment by travis.illig on 4 Jan 2014 at 12:06

GoogleCodeExporter commented 8 years ago
Did you consider the solution of using WeakReference in the implementation of 
the Disposer? It seems like would resolve the issue without any change to the 
API.

Original comment by mike.ade...@gmail.com on 6 Jan 2014 at 1:03

GoogleCodeExporter commented 8 years ago
Yes, I did consider that. The concepts in this issue fairly strongly overlap 
with Issue #397, "Nested lifetime scopes aren't disposed when the parent is 
disposed":
https://code.google.com/p/autofac/issues/detail?id=397

Just switching to WeakReference doesn't fix it; it changes fundamentally the 
way you need to track activated instances and clean up after them. It adds a 
lot of complexity overall and from a cost/benefit standpoint isn't necessarily 
worth it. Check out the chain in issue #397 for some insight there.

We are looking at a lot of different ways to update the internals of Autofac 
for future releases - from performance to functionality - and may fix this as a 
byproduct of those improvements, but for now we're not looking to switch this 
up.

Original comment by travis.illig on 6 Jan 2014 at 3:39