C# IDisposable pattern and throwing ObjectDisposedException

Question

A question for you all.

In my company, we are developing an application that runs inside Microsoft’s MVC framework. The controller classes that we are implementing inherit from the MVC base class Controller. Example:

public class MyController: Controller
{
    protected bool IsDisposed { get; set; }
… various methods…
 }

The discussion we are having in the team centers around the Dispose() pattern. Essentially, this involves implementing the IDisposable interface, preferably according to the Microsoft-endorsed pattern.

See for example this link: http://msdn.microsoft.com/en-us/library/fs2xkftw%28v=vs.110%29.aspx

Interestingly, our controller classes do not own any resources, either managed or managed. As a result, the implementation of Dispose(bool) is greatly simplified:

protected override void Dispose(bool disposing)
{
    IsDisposed = true;
    base.Dispose(disposing);
}

There is some disagreement about the use (or need) of the IsDisposed property, that is used in the following method:

protected void ThrowIfDisposed()
{
    if (IsDisposed) throw new ObjectDisposedException(“MyController”);
}

This method is then called at the beginning of every method that does "real" work. The thinking here is that a disposed object should not be used again, hence it should throw the ObjectDisposedException. The other opinion is that, since Dispose(bool) does “nothing” (other than setting the IsDisposed property and calling Dispose(bool) on the base class), the “disposed” object is not really in a state that is unusable, so there is no reason to throw. Therefore, there is no reason to even implement Dispose(bool).

An argument against this is that MyController should throw when it is disposed and one of its methods is called, so that its behaviour does not change should in future versions managed and/or unmanaged resources be added.

The argument against this last point is that MyController should never add any resources in future versions, but rather it should be derived from should the need to add resources occur in the future. An additional question is: why doesn’t the (library) class Controller implement ThrowIfDisposed() or something similar?

So, summarizing, faction one wants to implement Dispose(bool) and ThrowIfDisposed() as shown above, faction two thinks they are unnecessary and wants to do away with them.

I see merits in both viewpoints, cannot really make up my mind. Opinions?

Answer 1

Interestingly, our controller classes do not own any resources, either managed or managed.

Then you do not need the IDisposable pattern.

This method [ ThrowIfDisposed() ] is then called at the beginning of every method that does “real” work.

The qustion here is: Why?

If you do want to track a usable/abandoned state then just don't call it IsDisposed.

why doesn’t the (library) class Controller implement ThrowIfDisposed() or something similar?

Because it isn't useful.

Go back to the beginning: why did somebody think this was necessary? What is it for?
It seems you can just rip it out.

Answer 2

If an object receives a request after being disposed, the request should not fail for any reason other than ObjectDisposedException--meaning it shouldn't cause a NullReferenceException, yield garbage data, crash the system, or do any other such thing. That does not imply that the request shouldn't succeed--merely that ObjectDisposedException should be the only mode of failure.

As for IsDisposed, I can't really see much use for it in most cases. Code which doesn't know whether an object is disposed generally won't know much else enough about the object's state to know whether it might be unusable for some other reason; consequently, code that doesn't know whether an object is disposed generally shouldn't do anything with it except--in some cases--to Dispose it, and in most of those cases where one would want to call Dispose on an object if it hadn't already been disposed, one should simply call Dispose unconditionally. There could be some value in having a method MayNeedDisposing which would return true for objects with non-trivial Dispose methods until they've been disposed, but such a method could unconditionally return false for objects with null Dispose methods [in cases where a class would e.g. register a callback purely for the purpose of calling Dispose on something, testing MayNeedDisposing beforehand could be a useful optimization].

Finally, with regard to whether a class or interface should implement or inherit IDisposable, the issue is not whether the type itself acquire resources, but rather whether the last surviving reference to something that does acquire resources may be stored in that type. For example, even though the vast majority of IEnumerator<T> implementations don't acquire resources, some do, and in many such cases the only references to such objects will be stored in variables of type IEnumerator<T>, held by code that has no idea whether the objects in question have any resources or not. Note that non-generic IEnumerator doesn't implement IDisposable but some implementations acquire resources. As a consequence, code which calls IEnumerable.GetEnumerator will be required to call Dispose on the returned enumerator if it implements IDisposable. The fact that IEnumerator doesn't inherit IDisposable doesn't absolve code which receives an IEnumerator implementation that also implements IDisposable from the responsibility of calling Dispose--it merely makes it harder for such code to carry out that responsibility.

Answer 3

Faction two is absolutely right. Don't do it.

Implementing IDisposable in the way described and for the reasons given is a violation of at least three good principles.

1. You ain't gonna need it. http://c2.com/cgi/wiki?YouArentGonnaNeedIt
2. Do the simplest thing that could possibly work. http://www.xprogramming.com/Practices/PracSimplest.html
3. Keep it simple stupid. https://en.wikipedia.org/wiki/KISS_principle

It's just wrong on every level. Don't do it.