CODEWITHSUNDEEP
.netgarbage-collectionidisposablefinalizersuppressfinalize
Kanini
Kanini

Reputation: 1993

Should I implement GC.SupressFinalize on IDisposable AND Finalize?

The code review checklist in my new client place has the following -

Class implementing Dispose and Finalize should have a call to GC.SupressFinalize in Dispose implementation

Why?

Should it not read as Class implementing IDisposable interface should have a call to GC.SupressFinalize in the Dispose implementation?

Or Am I missing something silly?

Upvotes: 2

Views: 504

Answers (2)

Hans Passant
Hans Passant

Reputation: 941455

It's accurate. If the Dispose(bool) method did its job then there is no longer any point to let the finalizer do it again. Calling GC.SuppressFinalize() is an optimization, you stop .NET from bothering to call a finalizer that does nothing.

I noticed that you wrote Class with a capital C. That's a hint that you are writing your code in VB.NET. Watch out, the IDE does the wrong thing in 99.99% of all cases. As soon as you press Enter after typing "Implements IDisposable", it inserts the wrong code:

    Private disposedValue As Boolean = False        ' To detect redundant calls

    ' IDisposable
    Protected Overridable Sub Dispose(ByVal disposing As Boolean)
        If Not Me.disposedValue Then
            If disposing Then
                ' TODO: free other state (managed objects).
            End If

            ' TODO: free your own state (unmanaged objects).
            ' TODO: set large fields to null.
        End If
        Me.disposedValue = True
    End Sub

#Region " IDisposable Support "
    ' This code added by Visual Basic to correctly implement the disposable pattern.
    Public Sub Dispose() Implements IDisposable.Dispose
        ' Do not change this code.  Put cleanup code in Dispose(ByVal disposing As Boolean) above.
        Dispose(True)
        GC.SuppressFinalize(Me)
    End Sub
#End Region

Yuck. That's the boilerplate implementation of a finalizer, well documented in the MSDN Library btw. It's wrong. It is extremely rare to actually need a finalizer, the .NET classes already take care of it themselves. If you really do use an operating system handle then you should use one of the SafeHandle derived classes. Or write your own wrapper.

Edit it back to this:

Public Sub Dispose() Implements IDisposable.Dispose
    someField.Dispose()
    '' maybe some more
    ''...
End Sub

Upvotes: 1

Jon Skeet
Jon Skeet

Reputation: 1500505

You're missing the fact that not every disposable class needs a finalizer - in fact, very few do, particularly due to .NET 2.0's SafeHandle type. If there's no finalizer, why would you need to call SuppressFinalize?

Upvotes: 6

Related Questions