Don't implement IDisposable on Controller types

[dotnetjunkie]

The Microsoft.AspNet.Mvc.Controller type implements IDisposable, while it doesn't do any disposing itself. This causes all controller implementations (in case they derive from Controller, which they don't have to do anymore, which is awesome btw) to be registered for disposal, and be disposed at the end of the request, while under normal conditions controllers have nothing to dispose. In the common case, any disposable resource is hidden behind some service that is injected into the controller.

In case developers need to dispose resources from within their controller class, they can implement IDisposable themselves and the framework can still make sure that controllers are disposed.

[ploeh]

While I haven't looked at the Controller class source code, if it's true that it doesn't have anything to dispose of itself, it shouldn't implement IDisposable.

I like how @nblumhardt put it almost six years ago:

an interface [...] generally shouldn't be disposable. There's no way for the one defining an interface to foresee all possible implementations of it - you can always come up with a disposable implementation of practically any interface.

Just replace the term interface with abstract base class - it makes no difference.

Implementation of IDisposable is purely a concrete concern; it has no place in an abstraction.

[dotnetjunkie]

While I haven't looked at the Controller class source code

Here it is:

public void Dispose()
{
    Dispose(disposing: true);
    GC.SuppressFinalize(this);
}

/// <summary>
/// Releases all resources currently used by this <see cref="Controller"/> instance.
/// </summary>
/// <param name="disposing"><c>true</c> if this method is being invoked by the <see cref="Dispose"/> method,
/// otherwise <c>false</c>.</param>
protected virtual void Dispose(bool disposing)
{
}

[Galilyou]

I agree with this. There isn't any requirement on controller class definitions any more, so why force the disposable implementation?

[itomek]

But if you have a resource that you want to dispose when the controller's life ends, such as a database context, wouldn't this be very handy for you to rely on the framework for this?

[dotnetjunkie]

@itomek, what you should do in that case is implement IDisposable on your Controller instead of overriding the virtual Dispose method.

So with the current design of MVC, your code will look like this:

public sealed class MyController : Controller
{
    protected override void Dispose(bool disposing) {
        try {
            base.Dispose(disposing);
        } finally {
            // Your dispose code here
        }
    }
}

Do note that, in case your code lacks the call to base.Dispose or the try-finally block, your implementation if flawed, so this is an implicit requirement, but this is very often forgotten.

With the alternative design where Controller doesn't implement IDisposable, your code will instead look as follows:

public sealed class MyController : Controller, IDisposable
{
    public void Dispose() {
        // Your dispose code here
    }
}

Since there is no base.Dispose we can't forget to call it, and because of the lack of a base.Dispose we can't forget to wrap our code in a try-finally. I hope you agree that it actually becomes much simpler for everyone when the Controller base class does not implement IDisposable. The only thing the framework must ensure is that controllers are disposed when their lifetime ends (which might be at a different moment in time than at the end of the request btw).

[itomek]

Hmm, so what would call the Dispose method in the alternate design?

[dotnetjunkie]

@itomek: The same piece of code that is calling Controller.Dispose in the current design: the framework.

[itomek]

ok. thanks.

[dotnetjunkie]

The ViewComponent base class doesn't implement IDisposable. Since view components are 'mini-components' (according to the documentation), they are expected to have the same usage pattern as controllers do. So if view components don't need IDisposable by default, neither do controllers.

[Eilon]

We implemented IDisposable on Controller for compatibility with MVC 5 and earlier.

We agree that it would have been fine to not have it implemented there and leave it up to individual controllers to choose to implement IDisposable on their own (though that can get a tiny bit tricky due to needing to "hide" the Dispose method from the action invoker).

In the case of ViewComponents they don't implement IDisposable because there's no back-compat concern.

[ploeh]

@Eilon Taking into account how much this new version of ASP.NET already breaks compatibility, I don't find that argument compelling...

[Eilon]

@ploeh we focused a lot on application-level source code compatibility for MVC 5 applications. We've preserved a lot of names and patterns to ensure compatibility. The decision is always made on a case-by-case basis and we have to weigh the costs and benefits. In this particular case the benefit seemed minimal, and the cost seemed higher.