views:

1107

answers:

5

I use a default IDisposable implementation template (pattern) for my code.

snippet:

public void Dispose()
{
    Dispose(true);

    GC.SuppressFinalize(this);
}

protected virtual void Dispose(bool isDisposing)
{
    if (!this.disposed)
    {
        if (isDisposing)
        {
            //cleanup managed resources
        }

        //cleanup unmanaged resources

        this.disposed = true;
    }
}

My question: why is the call "GC.SuppressFinalize(this)" in the Dispose public method? I would place "GC.SuppressFinalize(this)" in the "if (isDisposing)" section of the protected method, after disposing managed resources.

Like this:

protected virtual void Dispose(bool isDisposing)
{
    if (!this.disposed)
    {
       if (isDisposing)
       {
           //cleanup managed resources

           GC.SuppressFinalize(this);
       }

       //cleanup unmanaged resources

       this.disposed = true;
    }
}
A: 

The idea is that your cleanup code should only be called once. However there are two points of entry: the Dispose method and object finalizers. When Dispose is called, you opt-out of finalization so that your cleanup code is only called once. The code here might illustrate it better.

Quote:

// NOTE: Leave out the finalizer altogether if this class doesn't 
// own unmanaged resources itself, but leave the other methods
// exactly as they are.
Ken Browning
My example is an complex base class structure with IDisposable. I have searched many scenario's with IDisposable implementations (templates), but didn't find a correct base-class -> derived class example.
Patrick Peters
The standard MSDN pattern includes a derived class - that's why Dispose(bool) is virtual.
Henk Holterman
it is not a complete example @ MSDN: MSDN shows only the base-class implementation and not a base-class/derived class example.
Patrick Peters
+1  A: 

I think either layout could have been chosen, but probably they wanted to emphasize "put all deallocation code in this method" in the protected Dispose method, so they put the other artifact of disposing (Suppressing finalization) elsewhere.

Also, suppose a derived class had another reason for calling the protected Dispose method, but did still want finalization to occur (for whatever imagined reason, I don't know).

Damien_The_Unbeliever
But what if I call the public Dispose of several times (because that should be possible according the guidelines), the suppress call is also called several times. That shouldn't be the case... In my code example it will be called just once.
Patrick Peters
Although Dispose *can* be called several times, it's generally a symptom of problems elsewhere in the codebase - two or more pieces of code beliefing that they control the life of the object. Similarly, GC.SuppressFinalize *can* be called multiple times.
Damien_The_Unbeliever
Though it could be flaw in code when multiple calls of Dispose is called, it should be programmed defensively so that it can handle it.
Patrick Peters
Calling SuppressFinalize multiple times is not a problem. It is a sign of problems elswhere though.
Henk Holterman
+4  A: 

I suppose its a clear case of Template Design pattern.

Your abstract class is Designed to take care of all important/necessary tasks required (Here, GC.SuppressFinalize(this)), and allowing a derived class to override only some part of the code.

There are 2 cases here:
Snippet 1, SuppressFinalize, in Dispose
Snippet 2, SuppressFinalize, in Dispose(true)

Here, Snippet 1, always makes sure that GC.SuppressFinalize is executed always. While snippet 2, leaves the execution of GC.SuppressFinalize at the mercy of derived class.

So, by putting GC.SuppressFinalize, in Dispose method, you as a designer of your class will always make sure that irrespective of whatever code written by derived classes, GC.SuppressFinalize will be executed.

This is only the benefit of writing SuppressFinalize in Dispose rather then Dispose(true).

nils_gate
I think nils_gate is very much on topic. Unless you dont cae about inheritance, but then edit your question.
Henk Holterman
When the derived class omits the base dispose call then more resources are not cleaned up...
Patrick Peters
Yes, or if a (hiding/new) Dispose() is implemented and called on a derived class. But that simply breaks the pattern. Solution: Don't Do It.
Henk Holterman
I think it is the responsibility of the derived class to implement the base calls correctly, so I still prefer my solution.
Patrick Peters
+1  A: 

Cause .Dispose is when managed code (your code) disposes of the object thereby opting out of finalisation. People usually create another route into Dispose(bool disposing) via a finaliser and the call wouldn't make any sense for the finaliser to make.

Quibblesome
+3  A: 

The Dispose(bool isDisposing) method isn't part of the IDisposable interface.

You would normally call Dispose(true) from your Dispose method, and call Dispose(false) from your finalizer, as a fallback in the case where the object hasn't already been disposed.

Calling SuppressFinalize tells the GC that there's no need to call your object's finalizer, presumably because all your cleanup was done when Dispose was called.

If you don't have a finalizer on your class, then you don't need to call SuppressFinalize at all, since there's no finalizer to suppress!

Joe Duffy has some great guidelines on disposal, finalization, garbage collection etc.

LukeH
Thanks for the info, but is this on-topic ? Look at my specific question, it's about the location of the suppress call.
Patrick Peters
The examples in your question don't have finalizers, and if that's also the case in your "real" code then calling SuppressFinalize is irrelevant/unnecessary wherever you do it, because there's no finalizer to suppress.
LukeH
It is indeed not part of the IDisposable interface but it is very much part of the Disposable implementation pattern. See MSDN
Henk Holterman
@Henk, Absolutely right, I should've made that clear in my answer. The point I'm trying to make is that *where* you call SuppressFinalize is *completely irrelevant* if your class doesn't have a finalizer in the first place!
LukeH
Luke, given the context, Peters class simply needs to have a destructor.
Henk Holterman
@Henk, Not necessarily. Finalizers are expensive. You should only implement a finalizer if you absolutely need to, and you shouldn't need to unless your class handles unmanaged resources directly.
LukeH
I assume he has unmanaged stuff - at least the comment is there. But you are right that a class that is only the owner of IDisposable objects should be Idisposable w/o a destructor.
Henk Holterman
The example I used here is with unmanaged resources in mind. When using unmanaged resources you must use a finalizer to cleanup the unmanaged stuff when the Dispose() call hasn't been used.
Patrick Peters