The IDisposable design pattern considered harmful

Discusses the pros and cons of the standard IDisposable design pattern used throughout the .NET framework base class library. Provides a blueprint for a better (IMHO) IDisposable design pattern.

Here is the MSDN code from the article (click to expand / collapse the code):

' Design pattern for the base class.
' By implementing IDisposable, you are announcing that instances
' of this type allocate scarce resources.
Public Class BaseResource
   Implements IDisposable
   ' Pointer to an external unmanaged resource.
   Private handle As IntPtr 
   ' Other managed resource this class uses.
   Private Components As Component
   ' Track whether Dispose has been called.
   Private disposed As Boolean = False

   ' Constructor for the BaseResource Object.
   Public Sub New()
      ' Insert appropriate constructor code here.
   End Sub

   ' Implement IDisposable.
   ' Do not make this method Overridable.
   ' A derived class should not be able to override this method.
   Public Overloads Sub Dispose()Implements IDisposable.Dispose
      Dispose(true)
      ' Take yourself off of the finalization queue
      ' to prevent finalization code for this object
      ' from executing a second time.
      GC.SuppressFinalize(Me) 
   End Sub

' Dispose(disposing As Boolean) executes in two distinct scenarios.
' If disposing is true, the method has been called directly 
' or indirectly by a user's code. Managed and unmanaged resources 
' can be disposed.
' If disposing equals false, the method has been called by the runtime
' from inside the finalizer and you should not reference other    
' objects. Only unmanaged resources can be disposed.
Protected Overloads Overridable Sub Dispose(disposing As Boolean)
   ' Check to see if Dispose has already been called.
   If Not (Me.disposed) Then
      ' If disposing equals true, dispose all managed 
      ' and unmanaged resources.
      If (disposing) Then
         ' Dispose managed resources.
         Components.Dispose()
      End If
      ' Release unmanaged resources. If disposing is false,
      ' only the following code is executed.      
      CloseHandle(handle)
      handle = IntPtr.Zero
      ' Note that this is not thread safe.
      ' Another thread could start disposing the object
      ' after the managed resources are disposed,
      ' but before the disposed flag is set to true.
      ' If thread safety is necessary, it must be
      ' implemented by the client.
   End If
   Me.disposed = true
End Sub

   ' This Finalize method will run only if the 
   ' Dispose method does not get called.
   ' By default, methods are NotOverridable. 
   ' This prevents a derived class from overriding this method.
   Protected Overrides Sub Finalize()
         ' Do not re-create Dispose clean-up code here.
         ' Calling Dispose(false) is optimal in terms of
         ' readability and maintainability.
         Dispose(false)
   End Sub
   
   ' Allow your Dispose method to be called multiple times,
   ' but throw an exception if the object has been disposed.
   ' Whenever you do something with this class, 
   ' check to see if it has been disposed.
   Public Sub DoSomething()
      If Me.disposed Then
         Throw New ObjectDisposedException()
      End if
   End Sub
End Class

' Design pattern for a derived class.
' Note that this derived class inherently implements the 
' IDisposable interface because it is implemented in the base class.
Public Class MyResourceWrapper
   Inherits BaseResource
   
   ' A managed resource that you add in this derived class.
   private addedManaged As ManagedResource
   ' A native unmanaged resource that you add in this derived class.
   private addedNative As NativeResource
   ' Track whether Dispose has been called.
   Private disposed As Boolean = False

   ' Constructor for the MyResourceWrapper object.
   Public Sub New()      
      MyBase.New()
      ' Insert appropriate constructor code here for the
      ' added resources.
   End Sub

   Protected Overloads Overrides Sub Dispose(disposing As Boolean)
      If Not (Me.disposed) Then
         Try
            If disposing Then
              ' Release the managed resources you added in
              ' this derived class here.
              addedManaged.Dispose()
            End If
            ' Release the native unmanaged resources you added
            ' in this derived class here.
            CloseHandle(addedNative)
            Me.disposed = true
         Finally
            ' Call Dispose on your base class.
            MyBase.Dispose(disposing)
         End Try
      End If
   End Sub
End Class
' This derived class does not have a Finalize method
' or a Dispose method without parameters because it 
' inherits them from the base class.

So what's wrong with the code?

There are two things why I don't like it.

The first one is the necessity to declare and maintain the disposed flag in each and every derived class.

The other reason why I don't like the code is very subjective and it is the naming of the Protected Dispose(Boolean) method and it's disposing parameter. After all, we are inside Dispose method, so the disposing flag should always be True, right? Wrong! When called by a user code (presumably inside in a Finally block), the flag is True. When called from the Finalize method, the flag is False. Not quite intuitive, IMHO.

So what can be done to improve the design patter for my likings?

The first thing I would suggest is to expose the private disposed flag through a protected property:

Private disposed As Boolean
...
Protected ReadOnly Property IsDisposed() As Boolean
Get
	Return disposed
End Get

Protected Sub CheckDisposed()
	If disposed Then
		Throw New ObjectDisposedException()
	End If
End Sub

Finally, in order to avoid to remember when disposing is True or False and what kind of resources to release, I would implement the following two methods in a base class:

Protected Overridable Sub DisposeManagedResources()
	...
End Sub

Protected Overridable Sub DisposeUnmanagedResources()
...
End Sub

Public Sub Dispose() Implements IDisposable.Dispose
	If disposed Then
		Return
	End If

	Me.DisposeManagedResources()
	Me.DisposeUnmanagedResources()
	
	disposed = True

	GC.SuppressFinalize(Me)
End Sub

Protected NotOverridable Sub Finalize()
	If Not disposed Then
		Me.DisposeUnmanagedResources()
	End If
End Sub

To wrap things up, let's summarize, what an inherited class implementer has to remember using both approaches.

The "classic" approach:

• Override the protected Dispose(Boolean) method and call MyBase.Dispose(disposing).
• Declare a private disposed flag and set it to True at the end of the Dispose(Boolean) method.
• Check the disposed flag where appropriate using the "three-liner" If disposed Then statement.

My improved approach:

• Override the DisposeManaged/UnmanagedResources as necessary and call the MyBase implementation in each overridden method.
• Check the disposed flag where appropriate using the "one-liner" CheckDisposed statement.

So what approach will you use? If you don't like my verbose methods and stick with the classic approach, I encourage you to use the CheckDisposed method at least. (I guess it's so obvious that most people already use some variation of the CheckDisposed method).

PS: Have you ever looked at the list of classes that implement IDisposable? There are 85 of them in the .NET framework BCL version 1.1. And for some of them I was very surprised to see they implement IDisposable! Have you written some GDI+ code and used the Matrix class? OK. Have you disposed all the matrix instances you've used? Not? It is likely then, your app is more resource intensive than it could be.

© Palo Mraz - June 14, 2003