42

I need some advice on the implementation of the Dispose method.

In our application the user designs their own UI. I have a preview window that shows what the UI is going to look like. All object drawn in this UI ultimately derive from a common base class ScreenObject. My preview manager contain a single object reference to a ScreenGrid which is the grid object for the entire preview area.

Question #1

Some of my derived screen classes hold onto unmanaged resources, such as a database connection, bitmap image and a WebBrowser control. These classes need to dispose of these objects. I created a virtual Dispose method in the base ScreenObject base class and then implemented an override Dispose method in each of the derived classes that hold onto unmanaged resources. However, right now I just created a method called Dispose, I am not implementing IDisposable. Should I implement IDisposable? If so how do I implement it?

  • Just on the derived classes that have unmanaged resources
  • The base class and derived classes that have unmanaged resources OR
  • The base class and all derived classes including those that do not have unmanaged resources

Is it wrong to put a virtual Dispose method in a base class that doesn't have unmanaged resources so that you can take advantage of polymorphism?

Question #2

In reading about the Dispose method and the IDisposable interface Microsoft states that the disposing object should only call the Dispose method for its parent. The parent will call it for its parent and so on. To me this seems backwards. I may want to dispose of a child but keep its parent around.

I would think it should be the other way around, an object being disposed should dispose of its children. The children should then dispose of their children and so on.

Am I wrong here or am I missing something?

Neuron
  • 5,141
  • 5
  • 38
  • 59
WPFNewbie
  • 2,464
  • 6
  • 34
  • 45
  • 4
    To your second question. Maybe you misunderstood something. Microsoft only says that the class should call its parent _class_ Dispose method, which means the method of the base class. I agree that the class only have to dispose its children and at the end its base. – MatthiasG Sep 20 '11 at 12:28
  • Do you hold unmanaged resources in your class? Or do you just own a class which in turn owns the unmanaged resources? I hope it's the second, else you will have to deal with fun stuff such as critical finalization. – CodesInChaos Sep 20 '11 at 12:30
  • 3
    Question #1: You should implement _IDisposable_ so that you (and others) can then use the `using` statement in C#. Having a _Dispose_ method without implementing _IDisposable_ doesn't really make sense, you could just call it _MyCleanUp_ as well. – Marc Sep 20 '11 at 12:32
  • Your class _ScreenObject_ could also have an **abstract** method _MyCleanUp_ to force its implementation. With good documentation it should be clear what implementators have to do. – Marc Sep 20 '11 at 12:35
  • 2
    This could well be the 100th duplicate of this question. Did you bother even looking at the list of similar questions? – H H Sep 20 '11 at 12:36
  • Yes I see lots of the same answers to the same question, but I specifically asked different questions. As in the implementation when dealing with derived class that hold unmanaged resources and my misinterpretation of calling dispose on parent object, not the parent class. – WPFNewbie Sep 20 '11 at 12:44
  • I was thinking the exactly that, why not jsut create a method like MyCleanUp. As for making it abstract, that wouldn't work as I have many derived classes without managed resources that do not have to perform clean up. If I implement IDisposable what all classes should implement it, the base class and my derived classes that hold onto unmanaged resources only? – WPFNewbie Sep 20 '11 at 12:46
  • The reference implementation on MSDN does cater for inheritance and everything else. Don't DIY here. – H H Sep 20 '11 at 13:13
  • @WPFNewbie: The purpose of IDisposable isn't to destroy objects--it's to ensure that they may be safely abandoned. Except when it would be absolutely impossible to do so, every object should be designed so that it may be abandoned safely if either (1) IDisposable.Dispose is called first, or (2) it doesn't implement IDisposable. A class requires custom cleanup but doesn't implement IDisposable doesn't fulfill either of the above. – supercat Oct 26 '11 at 20:17
  • possible duplicate of [Will the Garbage Collector call IDisposable.Dispose for me?](http://stackoverflow.com/questions/45036/will-the-garbage-collector-call-idisposable-dispose-for-me) – John Saunders Dec 13 '11 at 19:12

3 Answers3

44

Question 1: Implement IDisposable as well, using the following pattern:

public class MyClass : IDisposable
{
    bool disposed;

    protected virtual void Dispose(bool disposing)
    {
        if (!disposed)
        {
            if (disposing)
            {
                //dispose managed resources
            }
        }
        //dispose unmanaged resources
        disposed = true;
    }

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

Question 2: What Microsoft means is that a derived class calls dispose on it's parent class. The owner of the instance only calls Dispose on the most derived type.

An (shortened) example:

class Parent : IDisposable 
{
    protected virtual void Dispose(bool disposing)
    {
        if (!disposed)
        {
            if (disposing)
            {
                //dispose managed resources
            }
        }
        //dispose unmanaged resources
        disposed = true;
    }

}
class Child : Parent, IDisposable 
{ 
    protected override void Dispose(bool disposing)
    {
        if (!disposed)
        {
            if (disposing)
            {
                //dispose managed resources
            }
            base.Dispose(disposing);
        }
        //dispose unmanaged resources
        disposed = true;
    }

}
class Owner:IDisposable
{
    Child child = new Child();
    protected virtual void Dispose(bool disposing)
    {
        if (!disposed)
        {
            if (disposing)
            {
                if(child!=null)
                {
                    child.Dispose();
                }
            }
        }
        //dispose unmanaged ressources
        disposed = true;
    }
}

The owner only calls Dispose on the Child, but not on the Parent. The Child is responsible for calling Dispose on the Parent.

Neuron
  • 5,141
  • 5
  • 38
  • 59
PVitt
  • 11,500
  • 5
  • 51
  • 85
  • FxCop rules gives a sample implementation of this case: http://msdn.microsoft.com/en-gb/library/ms182330(VS.80).aspx – Ucodia Sep 20 '11 at 12:41
  • 1
    No, using the disposing pattern is wrong 99.9% of the time. Only used if the class has a finalizer, it shouldn't have one. Wrap unmanaged resources in their own class, like one of the SafeHandle derived classes. – Hans Passant Sep 20 '11 at 12:45
  • In your second answer, where would the declaration of the non virtual Dispose method with no parameters go? My thought is that you could put this in just the Parent class, or would both Parent and Child implement it? – WPFNewbie Sep 20 '11 at 12:51
  • Not sure what Hans is meaning, can you please elaborate for me? For example, I have a class ScreenPage which shows an HTML page on a portion of the screen. This is a WPF application and I am using the WebBrowser control to render the HTML page. My ScreenPage class is derived from ScreenParent. How are you suggesting that I dispose of the WebBrowser. Are you stating that I create a ScreenWebBrowser class, that holds the WebBrowser control and that my ScreenPanel just reference this class? If so, I don't see how that buys me anything, I still need to invoke Dispose. – WPFNewbie Sep 20 '11 at 12:56
  • I think what Hans means is that you should not implement a Finalizer unless you have something to finalize, i.e. unmanaged resources. Because every class that implements a finalizer is put into the garbage collector's finalizer queue, which results in the need of two GC runs to free the object. Without finalizer it would only need one GC run. – PVitt Sep 20 '11 at 13:32
  • @PVitt: I'd go even further and suggest that classes overriding Finalize (or including C# destructors) for purposes of cleanup should avoid holding strong references to any objects not needed for cleanup; if a class which would use unmanaged resources derives from a class which does not, the unmanaged resources should be encapsulated within their own class (which would thus be a managed resource) and the derived class should hold a reference to that. There's thus no reason to worry about an IDisposable implementation providing for a derived-class finalizer, since there shouldn't be any. – supercat Sep 20 '11 at 16:11
  • 1
    There is an error in that answer. Dispose method shouldn't be calling base.Dispose() - which is causing stack overflow. Instead it should call protected base object method, possibly base.Dispose(true) – rideronthestorm Sep 17 '13 at 08:36
  • 1
    I suggest moving `base.Dispose(disposing)` in the Child class outside of the `if (disposing)` logical block as the Parent class contains unmanaged resources and that block is reserved for freeing managed resources. If `Dispose(false)` is called on the Child then given the current implementation - the unmanaged resources that the Parent owns will not be freed. – Derek W Mar 09 '15 at 17:06
  • I don't understand why we need to call GC.SuppressFinalize(this). I mean, who cares if we do a redundant garbage collection? I we happen to forget something, maybe that is actually better than suppressing garbage collection. – Dabaus Nov 15 '21 at 10:27
8

Question 1:

Based on the types of objects that you list (i.e. Database, WebBrowser, Bitmap, etc.) these are only managed resources as far as .NET is concerned. Thus, you should implement IDisposable on any class that has disposable types as members. If they are locally declared instances, you just call 'using()' on them. While these instances you mention do have unmanaged resources under them, this is abstracted away from you by .NET thru the types you are using. Since you are only using managed types, you should implement IDisposable but without a finalizer. You only need to implement a finalizer if you truly have unmanaged resources as class members.

Question 2:

It seems that you are confusing inheritance (is a) with aggregation/containment (has a). For example, if "Container" contains a disposable resource as a class member, it is called aggregation/containment. So, calling base.Dispose() in the IDisposable implementation of Container has nothing to do with disposing of the disposable resource inside of Container. You should remember that if a class derives from Container, say "DerivedContainer", that it is an instance of Container albeit with additional members and/or functionality. So any instance of "DerivedContainer" has all of the members that its base class "Container" does. If you never called base.Dispose(), the disposable resource in "Container" would not be released properly (it would actually by the GC, but it's bad practice for many reasons to just 'let .NET take care of it') - please refer to my posted answer at Is it bad practice to depend on the .NET automated garbage collector?.

If you didn't call the base class Dispose(), you'd end up with a partially disposed object (disposed in the derived class but not in the base class) - a very bad scenario. So it is very important to call the base class Dispose().

I have a best practices pattern I've developed (with lots of experience and debugging memory dumps) written on my blog as an example. It shows how to implement IDisposable on a base class as well as a derived class:

http://dave-black.blogspot.com/2011/03/how-do-you-properly-implement.html

Community
  • 1
  • 1
Dave Black
  • 7,305
  • 2
  • 52
  • 41
  • The question was quite old by the time you responded to it. I took a look at your blog post, though. You seem to repeat the normal Microsoft pattern, defects and all. Among other things, rather than using one flag to provide thread-safe protection against multi-disposal for all derived classes, it requires a separate flag for every derived class and is not thread-safe. Also, since classes with finalizers should avoid holding objects not needed for finalization, derived classes shoudln't hold unmanaged resources if the base class does not. – supercat Dec 14 '11 at 00:15
  • I am confused by your comment... I responded only 3 months after the question was asked - surely that is not 'quite old'. My pattern is different from the Microsoft pattern. I have more safety precautions, include examples for disposing of unmanaged objects in the Dispose method, add comments about if you should be using a finalizer, comments about when you might need to implement thread synchronization. It is bad practice to include thread synchronization if you do not need to. Also, please enlighten me on the "defects" to which you refer. – Dave Black Nov 26 '12 at 18:26
1

I Implement IDisposable

 class ConnectionConfiguration:IDisposable
{
    private static volatile IConnection _rbMqconnection;
    private static readonly object ConnectionLock = new object();
    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }
    protected virtual void Dispose(bool disposing)
    {
        if (!disposing)
        {
            return;
        }
        if (_rbMqconnection == null)
        {
            return;
        }
        lock (ConnectionLock)
        {
            if (_rbMqconnection == null)
            {
                return;
            }
            _rbMqconnection?.Dispose();//double check
            _rbMqconnection = null;
        }
    }
}
SUNIL DHAPPADHULE
  • 2,755
  • 17
  • 32