views:

383

answers:

7

Let's take a look at the infamous IDisposable interface:

[ComVisible(true)]
public interface IDisposable
{
    void Dispose();
}

and a typical implementation, as recommended by MSDN (I omitted the check if current object has already been disposed):

public class Base : IDisposable
{
    protected virtual void Dispose(bool disposing)
    {
        if (disposing)
        {
            // release managed
        }
        // release unmanaged
        disposed = true;
    }

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

    ~Base()
    {
        Dispose(false);
    }
}

public class Derived : Base
{
    protected override void Dispose(bool disposing)
    {
        base.Dispose(disposing);
        if (disposing)
        {
            // release managed
        }
        // release unmanaged
        disposed = true;
    }
}

Problem is: I think this implementation is counter-intuitive. And it is also significantly different in base and derived class. Derived class is supposed to assume that base class implemented IDisposable properly and then override Dispose(bool), which is not even a part of the original interface.

I have to admit, I came up with this question because I usually ask junior programmers to implement IDisposable on a job interview. If they don't exactly know how it's supposed to be done, they come up with something close to this:

public class Base : IDisposable
{
    public virtual void Dispose()
    {
        // release managed and unmanaged
        GC.SuppressFinalize(this);
    }

    ~Base()
    {
        // release unmanaged
    }
}

public class Derived : Base
{
    public override void Dispose()
    {
        // release managed and unmanaged
        base.Dispose();
    }

    ~Derived()
    {
        // release unmanaged
    }
}

To me, this implementation is more clear and more consistent. Of course, the bad thing is that we have to release unmanaged resources in two different places, but the important point is that probably over 99% custom classes do not have anything unmanaged to dispose, so they won't need a finalizer anyway. I can't explain to a junior programmer why MSDN implementation is better because I don't really understand it myself.

So I'm wondering, what led to such unusual design decisions (making derived class to override a different method than the one in the interface and making him think about unmanaged resources which it most probably doesn't contain). Any thoughts on this matter?

A: 

My understanding is that the entire reason for IDisposable is to release unmanaged resources, so I'm confused why you're stating "99% custom classes do not have anything unmanaged to dispose" - if you're implementing IDisposable it should be because you have unmanaged resources.

MSDN IDisposable

BarrettJ
Wrong. If you have managed resources, you should still implement IDisposable and dispose them.
SLaks
Not always true. You may be encapsulating a type with native resources, or implementing a factored type.
Reed Copsey
@SLaks: If the resources are completely managed, it's less important (since a completely managed type will be handled by the GC correctly...)
Reed Copsey
Barret, the 'managed' resources are usually (always?) IDisposable objects, and for them `Dispose()` needs to be chained.
Henk Holterman
+1  A: 

MSDN Magazine has an article about this pattern.

This doesn't quite answer the question, but you can use the following code snippet to implement the pattern.

<?xml version="1.0" encoding="utf-8" ?>
<CodeSnippets  xmlns="http://schemas.microsoft.com/VisualStudio/2005/CodeSnippet"&gt;
    <CodeSnippet Format="1.0.0">
        <Header>
            <Title>Dispose pattern</Title>
            <Shortcut>dispose</Shortcut>
            <Description>Code snippet for virtual dispose pattern</Description>
            <Author>SLaks</Author>
            <SnippetTypes>
                <SnippetType>Expansion</SnippetType>
                <SnippetType>SurroundsWith</SnippetType>
            </SnippetTypes>
        </Header>
        <Snippet>
            <Declarations>
                <Literal Editable="false">
                    <ID>classname</ID>
                    <ToolTip>Class name</ToolTip>
                    <Default>ClassNamePlaceholder</Default>
                    <Function>ClassName()</Function>
                </Literal>
            </Declarations>
            <Code Language="csharp">
                <![CDATA[
        ///<summary>Releases unmanaged resources and performs other cleanup operations before the $classname$ is reclaimed by garbage collection.</summary>
        ~$classname$() { Dispose(false); }
        ///<summary>Releases all resources used by the $classname$.</summary>
        public void Dispose() { Dispose(true); GC.SuppressFinalize(this); }
        ///<summary>Releases the unmanaged resources used by the $classname$ and optionally releases the managed resources.</summary>
        ///<param name="disposing">true to release both managed and unmanaged resources; false to release only unmanaged resources.</param>
        protected virtual void Dispose(bool disposing) {
            if (disposing) {
                $end$$selected$
            }
        }]]>
            </Code>
        </Snippet>
    </CodeSnippet>
</CodeSnippets>
SLaks
+4  A: 

So I'm wondering, what led to such unusual design decisions (making derived class to override a different method than the one in the interface and making him think about unmanaged resources which it most probably doesn't contain). Any thoughts on this matter?

The main issue is that IDisposable was defined AFTER the framework was already designed and in existence. It's meant to handle a situation that managed code is trying to avoid - so it's really an edge case, if a very common one. ;)

This can be seen, btw, if you look at C++/CLI. It was designed after IDisposable, and as a result, implements IDisposable in a much more natural way (destructors [~ClassName]automatically become Dispose, and finalizers [!ClassName] are treated as finalizers).

The other issue is that IDisposable handles multiple situations. I wrote an entire blog series, walking through the different implementations that should be used when wrapping native code, encapsulating a class implementing IDisposable, and using it with factored types.

Technically, you only must implement the interface directly. The design decision to allow for a protected virtual void Dispose(bool disposing) method allows for extra flexibility that wouldn't be easily, and safely, handled in a public interface.

Reed Copsey
+10  A: 

I usually take out the guesswork for derived classes. Here's my .snippet file:

#region IDisposable pattern
/// <summary>
/// Dispose of (clean up and deallocate) resources used by this class.
/// </summary>
/// <param name="fromUser">
/// True if called directly or indirectly from user code.
/// False if called from the finalizer (i.e. from the class' destructor).
/// </param>
/// <remarks>
/// When called from user code, it is safe to clean up both managed and unmanaged objects.
/// When called from the finalizer, it is only safe to dispose of unmanaged objects.
/// This method should expect to be called multiple times without causing an exception.
/// </remarks>
protected virtual void Dispose(bool fromUser)
    {
    if (fromUser)   // Called from user code rather than the garbage collector
        {
        // Dispose of managed resources (only safe if called directly or indirectly from user code).
        try
            {
      DisposeManagedResources();
            GC.SuppressFinalize(this);  // No need for the Finalizer to do all this again.
            }
        catch (Exception ex)
            {
            //ToDo: Handle any exceptions, for example produce diagnostic trace output.
            //Diagnostics.TraceError("Error when disposing.");
            //Diagnostics.TraceError(ex);
            }
        finally
            {
            //ToDo: Call the base class' Dispose() method if one exists.
            //base.Dispose();
            }
        }
    DisposeUnmanagedResources();
    }
/// <summary>
/// Called when it is time to dispose of all managed resources
/// </summary>
  protected virtual void DisposeManagedResources(){}
/// <summary>
/// Called when it is time to dispose of all unmanaged resources
/// </summary>
  protected virtual void DisposeUnmanagedResources(){}
/// <summary>
/// Dispose of all resources (both managed and unmanaged) used by this class.
/// </summary>
public void Dispose()
    {
    // Call our private Dispose method, indicating that the call originated from user code.
    // Diagnostics.TraceInfo("Disposed by user code.");
    this.Dispose(true);
    }
/// <summary>
/// Destructor, called by the finalizer during garbage collection.
/// There is no guarantee that this method will be called. For example, if <see cref="Dispose"/> has already
/// been called in user code for this object, then finalization may have been suppressed.
/// </summary>
~$MyName$()
    {
    // Call our private Dispose method, indicating that the call originated from the finalizer.
    // Diagnostics.TraceInfo("Finalizer is disposing $MyName$ instance");
    this.Dispose(false);
    }
#endregion
Will
+1 This is the way I usually do it too.
SwDevMan81
You didn't call GC.SuppressFinalize(this);
Sergej Andrejev
@Sergej you're right; this is only part of the whole thing. I've updated my answer with the current contents of my .snippet file, which is an amalgamation of different snippets out there with personal touches
Will
Now I can vote it up :)
Sergej Andrejev
A: 

One problem I see with your implementation is that there is potention for the derived class not to call the base class Dispose method. In that case, the GC.SuppressFinalize might not get called when it should, and you would end up also calling the Finalizer. I like Will's solution to ensure that GC.SuppressFinalize is called. The recommended way by MSDN has a similiar feel and ensures that GC.SuppressFinalize is called if the object is Disposed by the developer.

SwDevMan81
+2  A: 

The answer to this and most other API design questions can be found in this book.

Framework Design Guidelines: Conventions, Idioms, and Patterns for Reusable .NET Libraries http://www.amazon.com/gp/product/0321545613?ie=UTF8&amp;tag=bradabramsblo-20&amp;link_code=wql&amp;camp=212361&amp;creative=380601

This is literally the set of rules Microsoft employees use to build .NET APIs. The rules are free (see below), but the book has the commentary that explains the rules. It really is a must have for .NET developers.

http://msdn.microsoft.com/en-us/library/b1yfkh5e.aspx

Jonathan Allen
Also mentioned in Effective C# book
Lex Li
+2  A: 

I'd say this is better:

public class DisposableClass : IDisposable {
  void IDisposable.Dispose() {
    CleanUpManagedResources();
    CleanUpNativeResources();
    GC.SuppressFinalize(this);
  }

  protected virtual void CleanUpManagedResources() { 
    // ...
  }
  protected virtual void CleanUpNativeResources() {
    // ...
  }

  ~DisposableClass() {
    CleanUpNativeResources();
  }
}
Jordão