Dispose(ohNotThisNonsenseYetAgain)

For some odd reason, I’ve been authoring quite a few disposable types lately. Under normal circumstances, this would hardly be worth mentioning, but the recommended disposition pattern has been grating a raw nerve quite some time now. At best, it feels like a slightly nasty code smell to me. Due the volume of recent implementations, I’m starting to feel like I’m hanging out next to a sewer. I suspect that I’m starting to slip toward ignoring the guideline in favour of what I would consider a better practice, and I’m hoping that a little bit of venting might postpone that, so here it goes…


Just in case you’re not already familiar with it, here’s an outline of the implementation recommended in the .NET Design Guidelines:


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

protected virtual void Dispose(bool disposing)	// or private in a sealed class.
{
	if (disposing)
	{	
		// Clean up managed state here.
	}

	// Clean up unmanaged state here.
}

Seems a little odd, given that we don’t ever enter the Dispose(bool) method with a false parameter value, doesn’t it? So why bother? Well, once upon a time, we used finalizers to help clean up unmanaged state. A finalizable type ought to implement IDisposable, and the “Clean up unmanaged state here” bit in the above example should be run from both the finalizer and the Dispose() method. Ideally, we’d like to be able to write something like the following in a finalizable and disposable class:


~TheClass()	// In case you're not familiar with the syntax, this is the finalizer.
{
	// Clean up unmanaged state here.

public void  Dispose()
{
	// Clean up managed state here.

	this.Finalize();
	GC.SuppressFinalize(this);
}

Unfortunately, our code can’t invoke the finalizer directly, so we can’t call the finalizer from the Dispose() method in order to clean up the unmanaged state. To work around this, we use the Dispose(bool) method shown in the first example above, and we invoke it with a false value from the finalizer:


~TheClass()
{
	this.Dispose(false);
} 

So far, so good. However (very, very big “however”), starting in .NET 2.0, we’re not supposed to write finalizers anymore (or at least not for typical unmanaged resource clean-up). This means that we’ll never call Dispose(bool) with a false argument in the vast majority of our new disposable type implementations. In other words, there’s no practical benefit in implementing the recommended disposition pattern anymore, unless you happen to be working on one of the very rare cases where a finalizer is still required. For the vast majority of cases, where a finalizer is not required, the following would be functionally equivalent:


public virtual void Dispose()	// or non-virtual in a sealed class
{
	// Clean up managed resources here.

	GC.SuppressFinalize(this);
}

Now, let’s assume for the moment that you’ve never heard of the recommended disposition pattern. What might your naïve implementation of IDisposable.Dispose() look like? Probably an awful lot like that last example, except you would probably omit the GC.SuppressFinalize call (which wouldn’t actually matter if your type isn’t finalizable, which it shouldn’t be anyway) In other words, if the recommended disposition pattern didn’t involve a bunch of unnecessary code, naïve implementers would automatically do the right thing. This is known as the “pit of success”, and it is generally considered a good thing.


But let’s come back to you, the very well informed disposition author. You’re not naïve. You know how to implement the disposition pattern. Heck, you even have a lovely little snippet. Why should it bother you that the recommended pattern is more verbose than strictly necessary? It’s unfortunate, but every little bit of code we write is an opportunity for error. Every bit of extra complexity (including little things like a senseless redirection or one extra nested if block) increases the probability of introducing a bug and decreases the readability and maintainability of the code. It’s bad enough when things like this appear in one-off code. How much worse when we’re asked to systematically add unnecessary code as part of a recommended implementation pattern?


Would you accept the following in a code review, if the DoSomething(bool) method were never invoked with a false parameter value:


public void DoSomething()
{
	this.DoSomething(true);
}

private void DoSomething(bool allowDoing)
{
	if (allowDoing)
	{
		...
	}
}

If not, why accept a guideline that recommends the same thing? Well, obviously because it’s a guideline, and you’re a well-mannered developer who cares about exposing APIs that are consistent with what consumers have come to expect from their interactions with core .NET Framework code. But if the fact that it’s a guideline is the only reason you’re still following the recommended disposition pattern (or if you’ve already given up on following it), you might want to consider letting the folks responsible for the guideline know that you’re not too fond of it

7 thoughts on “Dispose(ohNotThisNonsenseYetAgain)”

  1. Hello Nicole, great to see you blogging again.

    I agree with your point : a class which does not have a finalizer should not use the “classic” Dispose pattern. And most classes should not indeed have finalizers.

    In the worst case, I’ve seen programmers add a finalizer that just called Dispose(false), just to justify the existence of the bool parameter to Dispose !

    I’d like to add that as you mention, GC.SuppressFinalize has no effect on a class which does not have a finalizer. So the correct implementation of Dispose() in the most frequent case (no finalizer) should not in fact include this cryptic call.

    Cheers,
    –Jonathan

  2. Jonathan: Actually, the GC.SuppressFinalize call isn’t trivial, although the reason is a bit subtle.  That said, the more I think about it, the more I lean toward omitting it from the simple disposition pattern for that very reason.

    The reason that it’s potentially meaningful is that adding a finalizer to an existing class is not a breaking change.  If the class to which a finalizer happens to be added is a base class of an existing disposable class, then it all of a sudden becomes important whether the Dispose method in the subclass invokes GC.SuppressFinalize or not.

    The base class Dispose method should not call GC.SuppressFinalize unless the base class unmanaged code clean-up is invoked from the subclass disposition.  This is never the case if the base class finalizer is added after the subclass ships, regardless of whether the base class simulateously adds any flavour of disposition implementation, and regardless of whether the subclass is using a simple or classic disposition implementation.

    That’s starting to sound quite convoluted, so here’s a relatively straightforward conclusion: a non-finalizable disposable class that inherits from a non-disposable class should not call GC.SuppressFinalize, regardless of which version of the disposition pattern is implemented.  This has to do with correctness, not style, and it means that the existing recommended disposition pattern is even more severely flawed than I thought…

  3. Alfred: I do have some FxCop stuff percolating on the back burner, but nothing relevant to backlogs. Are there any particular aspects of backlogs that you would like me to tackle?

    As for the MVP summit, it’s highly unlikely given that I’m not an MVP anymore. (It turns out that maternity leave is not a particularly productive time when it comes to community contributions. )

  4. I’ve been wondering about the bool parameter and SuppressFinalize but usually made the change anyway just to appease FxCop. I think now I will be turning off that rule. Thanks for the info, and glad to see you blogging again. I look forward to future posts.

  5. The dispose pattern is created to support inheritance from IDisposable classes. You can indeed simplify the pattern a lot if and only if you make your class sealed.

    For example, is GC.SuppressFinalize() required if I don’t have (or inherit) a Finalize() method? Yes, unless I’m sealed, because a derived class could add a Finalize() method.

Leave a Reply

Your email address will not be published. Required fields are marked *


*

You may use these HTML tags and attributes: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <strike> <strong>