What is this IDisposable doing for me?

Question

I am working on a "learning program" and using the Code Rush refactoring tool with my learning. With the latest update of Code Rush it has been recommending implementing IDisposable to my programs. I know what MSDN says about IDisposable and I have a real basic understanding of what it does but because I don't know all the implications of implementing it I have been ignoring the recommendation. Today I decided to learn more about it and went along with the recommendation.

This is what it added to my program.

class Program : IDisposable
{
    static Service _proxy;

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

    protected virtual void Dispose(bool disposing)
    {
        if (disposing)
            if (_proxy != null)
            {
                _proxy.Dispose();
                _proxy = null;
            }
    }

    ~Program()
    {
        Dispose(false);
    }

So my questions is this. Does that do everything I need to do to get the advantage of IDisposable or do I need to do something in code to make it work? I put a break points on that and never reached it through the debugger so either it was not needed or I am not using it the way it was intended. Can someone please shed some light on what this is doing for me or how I should use it so it does do something for me?

Cataclinal

It looks to be right. The idea of IDisposable is your object would reference a unmanaged resource. Since an unmanaged resource will not get cleaned up by the GC, the object needs to say.. "WAIT, I have to do some house cleaning before I get destroyed". Anytime your object uses unmanaged code/calls OR containts objects that implement IDisposable, then you should also implement IDisposable and call Dispose() on any objects that implement it and/or clean up your unmanaged code. – Surprisal

Do you have something that needs disposing? – Crosslink

The pattern they're using there is explained in the commented code here: msdn.microsoft.com/en-us/library/system.idisposable.aspx – Recurvate

Frankly, Code Rush is doing a very poor job here, in this case. This code suggestion could introduce very bad behavior - it's a bug in their suggestion. – Japheth

In any case, lose the destructor. – Linguist

@HenkHolterman: Why would he do that? If he removes the Deconstructor (I prefer to call it that more than a destructor) then Dispose won't be called except if he used the class in the using clause, or if he explicitely call the method. – Margit

@tipx: trace what Dispose(false) actually does. – Linguist

Guys, thank you for your notes. We will fix the Implement IDisposable feature as soon as possible. – Hystero

@HenkHolterman: The point is not his implementation of IDisposable. Many people already pointed out that in this case, it's useless anyways. My point however is that in a proper implementation, as shown in MSDN, the deconstructor must call Dispose, otherwise Dispose is useless. – Margit

@Tipx: wrong. A destructor is only needed for unmanaged resources, and they are better dealt with by a Safehandle. Dispose is very useful (essential) w/o a destructor. – Linguist

We'll fix this for the next update, and we'll also improve analysis on the field to make sure the field definitely owns its contents and they are not shared (referenced/passed to) anywhere else. – Now

An update for Code Rush was announced today 12/22/2011. The suggestion for the IDisposable went away after the update. Thank you to everyone who helped me understand what was going on there and thank you to Mark and Alex for addressing the issue so quickly. – Cataclinal

Answer 1

In this case, CodeRush is suggesting you implement IDisposable because your class encapsulates an IDisposable resource (it's seeing _proxy, though that's not entirely a good thing since it's static). Code Rush thinks that there is a type that you're using which should be explicitly cleaned up, but you're not providing a way to do it via your class.

That being said, IDisposable is tricky - and this is one case where the generated code is not really a good implementation (even if _proxy were an instance variable). I would recommend not using the destructor in this case. It will cause performance issues in the GC, and in this case, doesn't help with the safety, as the encapsulated resource should handle that the case where you forget to call Dispose() for you. For details, see my series on IDisposable, and, in particular, encapsulating an IDisposable class.

In addition, this class shouldn't implement IDisposable (unless there is some other reason to do so) given the code above, as the _proxy resource is static. Disposing a static resource from an instance is likely to cause problems, at least in a general case. (In this case, it's obviously not problematic, but it's not a good practice...) Typically, a static variable has a very different lifetime than an instance member, so automatically disposing of it would be inappropriate.

Japheth

Exactly right -- 99% of the time, you don't need a destructor with IDisposable because you're just encapsulating a resource that already has a destructor. – Abigail

@JoelCoehoorn It's even worse in this case, as _proxy is static - Disposing it is completely inappropriate. – Japheth

The finalizer is recommended just in case a descendent class needs to override the Dispose(bool) method with unmanaged cleanup. Of course the deriving class could just call Dispose(false) from its own finalizer, which gets around the problem, though it breaks slightly from the 'official' pattern. Personally, I prefer to mark the class as sealed and avoid the problem entirely. – Aborigine

@DanBryant Yes - It'd be very inappropriate, in my opinion, for a subclass to add an unmanaged resource directly. The resource it's using should be wrapped in its own managed handler, which eliminates that issue... If you were going that route, though, then it could do its own destructor, as you mentioned, which would be more "pay for play" – Japheth

@Dan Bryant: The "official" pattern would be for the derived class to add a finalizer which calls Dispose(false) if necessary, though I can't think of any case in which a class derived from a non-trivial base class should be adding a cleanup finalizer. It may perhaps be reasonable for a derived class to add a finalizer whose sole purpose is to squawk at the lack of a proper dispose, but I can't think of any scenario in which cleanup shouldn't be handled by an encapsulated class. – Atkins

Answer 2

In a properly-written program, at any given time, for every object that could possibly have a meaningful implementation of IDisposable, there will be some entity that is responsible for ensuring that IDisposable.Dispose will get called on that object sometime between the last "real use" of that instance and its ultimate abandonment. In general, if an object Foo is going to hold references to objects which implement IDisposable, at least one of the following scenarios should apply:

1. Some other object will also hold the reference for at least as long as Foo needs it, and will take care of calling Dispose on it, so Foo should let the other object take care of the Dispose.
2. The object holding the reference will be the last thing to use the IDisposable object in question; if Foo doesn't call Dispose, nothing else will. In that scenario, Foo must ensure that that other object's Dispose method gets called once it (Foo) is no longer needed, and before it is abandoned. The most idiomatic way to handle this is for Foo to implement IDisposable.Dispose, and for its Dispose method to call Dispose on the IDisposable objects to which it holds the last useful references.

There are some scenarios where a class designer might not know whether its class is going to hold the last useful reference to an IDisposable object. In some cases, this issue may be resolved by having a class constructor specify whether a IDisposable passed to the constructor is being "lent" or "given". Other cases may require the use of reference-counting wrappers or other complex techniques.

Atkins