Thursday, February 03, 2011

The MVC 3.0 IDependencyResolver interface is broken. Don’t use it with Windsor.

I’ve been spending today moving Suteki Shop to MVC 3. One of the cool new features of MVC 3 is a much greater focus on allowing the various components (Controllers, Views, Filters etc) to be composed by an IoC container. It’s been possible since MVC 1 to provide your own custom controller factory, but the promise with MVC 3 is that you can provide an implementation of IDependencyResolver that wraps your IoC container of choice, and no longer have to bother about implementing various factory classes.

Here’s IDependencyResolver:

public interface IDependencyResolver {
object GetService(Type serviceType);
IEnumerable<object> GetServices(Type serviceType);
}

But there’s a huge problem with this. Can you spot it? That’s right, no ‘Release’ method. You can provide a service from your IoC container, but there’s no way to clean it up. If I was going to use this in Suteki Shop, I would have a memory leak of epic proportions.

According to the MVC team, this isn’t an issue because they will call ‘Dispose’ on any service that implements IDisposable, but that won’t work in many cases. Let me explain. Say I’ve got a controller like this:

public class HomeController : IController
{
private readonly ICustomerRepository customerRepository;
public HomeController(ICustomerRepository customerRepository)
{
this.customerRepository = customerRepository;
}
// ... does some stuff with customerRepository
}

My controller doesn’t implement IDisposable. So no Dispose method gets called.
 
Now say this is my implementation of ICustomerRepository:
 
public class CustomerRepository : ICustomerRepository, IDisposable
{
// ... etc
}

CustomerRepository does implement IDisposable. But the MVC framework doesn’t know this, indeed my HomeController doesn’t know it either. That’s the whole point of using an IoC container in the first place, it worries about component lifecycles, not the client service. If I resolve HomeController from Windsor it will create all its dependencies and if it finds any that implement IDisposable it will track the entire dependency graph. If I then call ‘Release’ on my HomeController instance, Windsor will know to dispose of CustomerRepository correctly. But if Release is never called CustomerRepository will never get disposed and my web application will gradually consume more and more memory, database connections, file handles, or whatever should have been cleaned up when Dispose gets called.
 
Krzysztof Kozmic has a great post here about how Windsor tracks components. It’s a ‘must read’ if you’re a Windsor user.
 
So the upshot is that all this nice new work in MVC3 is unusable, certainly with Windsor. It’s also a very dangerous trap for the unwary. My advice is that you shouldn’t implement an IDependencyResolver for Windsor. I don’t know how other IoC containers handle releasing components deep in their dependency graph, but I would make sure you understand exactly how they work before going down this route.

13 comments:

AdriaanP said...

Mike

You could use the PerWebRequest lifecycle in Windsor, that will release all components configured for the this lifecycle.

Ken Egozi said...

Maybe you can set up Windsor to return a Proxy to the controller, which also implements IDisposable, and delegates the Dispose call to container.Release. I have very little time today so I won't be able to actually create the code for this, but I bet that if you take it to the user-group, the good people there (probably Krzsistof) will come up with this in no time

Mike Hadlow said...

Hi Adraan, Ken,

Sure there are workarounds for this problem. As you guys say, you could register all your components as PerWebRequest, create some funky controller decorator, or another suggestion is to use a per-web-request child container.

But this is silly. Why should we have to think up creative workarounds for a feature of MVC3 that's supposed to enable container use simply because the MVC team couldn't provide a simple release mechanism.

My short term plan is to simply stick with the controller factory. My long term plan is look long and hard at open source web frameworks. The MVC team just doesn't seem to get it.

fschwiet said...

Interesting point. Doesn't CommonServiceLocator have this problem too though? Krzysztof's post on lifetime was an eye opener for me, so I can't get angry at the MVC team for missing this.

If using per-webrequest lifetime for anything that MVC loads is sufficient, I think I'd be fine with that as a work-around for now. But I'd like a windsor expert to confirm that is generally sufficient.

I think there might be other problems... For instance, what happens if you register a controller class with singleton lifetime, and that controller implements IDisposeable? It seems like MVC3's auto-dispose logic would not honor the intended lifetime there either.

Anonymous said...

Mike,

What is your controller doing that it needs to implement IDisposable?

Jeff Schumacher said...

What I don't understand is if your container is instantiating CustomerRepository and injecting it into the HomeController, the container should be disposing of your CustomerRepository just as it normally would... despite the IDependencyResolver stuff... right?

andy said...

Can't you configure your services as ephemeral and get a new instance per instantiation? Then GC will catch-up with unreferenced objects are normal?

Or if your services are stateless, configure then as a singleton and re-use the same instance everywhere?

(Don't use Windsor myself, but most containers seem to have some sort of configuration along these lines)

Am I missing something important here?

The only gotcha I can see is if you are performing work in Dispose(), e.g. a commit on a database. Dangerous territory!

Mike Hadlow said...

Hi Fschwiet,

Yes, as I said above there are workarounds, but it's irritating that we need workarounds when the whole point of the new MVC features was to enable better IoC integration. Apparently the MVC team were told about the problem, but did nothing to fix it. I assume it was because they would have to make their own internal service locator more sophisticated in order to deal with the changed semantics? But then don't make a song and dance about MVC being container friendly when it's not.

As you point out, the whole point of having a container is that it is responsible for the component life-cycle. Simply calling Dispose on a component violates that basic principle.

Hi Anonymous,

My controller doesn't implement IDisposable, that's part of the point :)

Hi Jeff,

The container needs some signal that it's time to dispose CustomerRepository 'as it normally would', but without the Release call it never gets it.

Hi Andy,

I do mostly configure my components as Transient in web applications. However, if a component, or any of its dependency chain implements IDisposable, Windsor will keep a handle on it and the GC won't collect it. Krzysztof's post that I referenced explains this point very well. It's a Windsor 101 point: if you resolve a component, you must release it as well.

Pete said...

It's not like they didn't know about it in advance... http://bradwilson.typepad.com/blog/2010/07/service-location-pt1-introduction.html (see comment trail)

hwiechers said...

I had some similar comments about IControllerActivator.

http://bradwilson.typepad.com/blog/2010/10/service-location-pt10-controller-activator.html

See near the bottom of the comments.

Anonymous said...

We do not use dependency resolver yet in our MVC project, but Windsor is used for other parts of the project. Here is how we compensate for lack of Release method in similar cases.
Define lifetime policy:

[Serializable]
public class TrulyTransientReleasePolicy : LifecycledComponentsReleasePolicy
{
public override void Track(object instance, Burden burden)
{
ComponentModel model = burden.Model;

if (model.LifestyleType == LifestyleType.Transient)
return;

// we skip the tracking for object marked with our custom Transient lifestyle manager
if ((model.LifestyleType == LifestyleType.Custom) &&
(typeof(TrulyTransientLifestyleManager) == model.CustomLifestyle))
return;

base.Track(instance, burden);
}
}

And then simply assign:
ioc.Kernel.ReleasePolicy = new TrulyTransientReleasePolicy();

Anonymous said...

Sorry accidently posted not completed post early.
PS: I do not remember where we got this code from. It basically redefines Trasient LifeStype Policy so IoC does not keep track of it internally. TrulyTransientLifestyleManager can be leaft out from method above. Unless you want to define it ans do some custom things with it.

Anonymous said...

end of tenancy cleaning You are a smoker? Well then you do not mind the tobacco smell in your apartment, but what if you expect guest who do not smoke at all? Smoking is really endangering your heath no matter if you are a real or passive smoker. The smoke is also very unhealthy for small children and pets, because they inhale of those bad smoke compounds that the cigarette contain. Statistics shows that people who inhale all the bad elements are not the smokers, but the passive one’s. You need to be very careful. post tenancy cleaning