2

The suggested dispose pattern from Microsoft says that Dispose() and finalizer should both call a virtual third method Dispose(bool). So it looks something like this:

public class DisposeBase : IDisposable
{
    private bool _Disposed = false;

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

    ~DisposeBase()
    {
        Dispose(false);
    }

    protected virtual void Dispose(bool disposing)
    {
        if (!_Disposed)
        {
            if (disposing)
            {
                /* Get rid of managed resources */
            }

            /* Get rid of unmanaged resources */

            _Disposed = true;
        }
    }
}

Derived classes would override Dispose(bool). I thought about restructuring it a bit like this:

public abstract class ExtendableResourceHandlerBase : IDisposable
{
    private bool _Disposed = false;

    /* private resources managed and unmanaged */

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

    ~DisposeBase()
    {
        Dispose(false);
    }

    private void Dispose(bool disposing)
    {
        if (!_Disposed)
        {
            if (disposing)
            {
                ManagedDispose();

                // Dispose of own managed resources
            }

            UnmanagedDispose();

            // Dispose of own unmanged resources

            _Disposed = true;
        }
    }

    protected abstract void ManagedDispose();

    protected abstract void UnmanagedDispose();

    protected abstract xxx ExtendMe(....)

    // other member functionality
}

I am thinking of scenario where in a framework you are declaring an abstract base class that provides an interface and some implementation aquiring resources that need to be disposed - hence the IDisposable interface. Now clients extending this base class would be forced to think about the disposal of their managed and unmanaged resources as well. In the case of the suggested pattern from microsoft one might forget about it. Please think of the name ExtendableResourceHandlerBase as just a place holder.

In my opinion this would make it easier for clients deriving from DisposeBase to implement their dispose methods. And as answers of another question show, other people think so too. The only reason I can think of why the good folks at microsoft build their pattern as it is now, is not to split up the disposal of managed and unmanaged resources. Are there any other reasons for it? Thanks alot for enlightening me.

Community
  • 1
  • 1
S. Larws
  • 363
  • 2
  • 8
  • I wrote an answer about placing IDisposable implementation at the base of your hierarchy here http://stackoverflow.com/a/874782/14357 . – spender Aug 05 '13 at 20:18
  • You need to describe benefits of your pattern. So far I see need to implement 2 functions and worry about chaining virtual 2 functions instead of 1... Not sure yet what balances it on positive side. – Alexei Levenkov Aug 05 '13 at 20:34
  • 1
    You only get one shot at inheritance, so this would be a no-no for me. Besides, I can't think of any real-world scenario where a derived class would be an "is-a DisposeBase". The whole purpose of the DisposeBase class is to dispose, so a derived class would be entirely about disposing itself in a different manner. – Keith Payne Aug 05 '13 at 20:38
  • 1
    Yep. I think Liskov wouldn't approve. – spender Aug 05 '13 at 20:44
  • @KeithPayne - original sample is "pattern" (in sense of "we recommend you implementing `IDispose` in your classes this way") - it does not consume your base class - just suggestion how to structure it (if indeed you need to dispose objects in your hierarchy). It is unclear why OP's suggestion would be better pattern for that (as it is more complicated...) – Alexei Levenkov Aug 05 '13 at 21:11
  • @Keith Payne - I completely agree with you that this `DisposeBase` should not be the base class of anything, I tried to simplify it, but I think I couldn't make myself clear, so I will edit it :). – S. Larws Aug 06 '13 at 15:00
  • @AlexeiLevenkov - I edited the question and hopefully made myself clear. – S. Larws Aug 06 '13 at 15:12
  • @spender - I hope the edit helped to satisfy Liskov :) – S. Larws Aug 06 '13 at 15:12

1 Answers1

5

When Microsoft documented and recommended the Dispose pattern, it was expected that classes which implemented IDisposable would often have one set of resources they would clean up in Dispose and a smaller set they clean up in Finalize; the pattern was designed around that expectation. In practice, the only classes which should ever override Finalize for any purpose other than to log failures to call Dispose are those which inherit directly from Object. If a derived class whose parent doesn't override Finalize would use an unmanaged resource that should be cleaned up in a finalizer, it shouldn't override Finalize itself nor hold the unmanaged resource directly, but instead encapsulate the resource in an instance of a class dedicated to that purpose. This will help avoid a lot of weird and tricky edge cases that can otherwise arise with finalization of objects which hold a mixture of managed and unmanaged resources.

Having the interface method which simply chains to a virtual method is a good idea, even if the protected void Dispose(bool) method will never get called with its parameter being anything other than true. The pattern might be a little cleaner if the protected virtual method were differentiated by name rather than by a useless parameter, but enough people expect the Microsoft pattern that one may as well use it even if it's not perfect.

supercat
  • 77,689
  • 9
  • 166
  • 211
  • +1. A great and ongoing source of confusion is the naming of the parameter - `disposing`. It would be so much clearer if MS had published the same pattern with the parameter named `isNotBeingCalledFromTheFinalizer` or something similar. But it is too late for that. – Keith Payne Aug 06 '13 at 12:43
  • @KeithPayne Would changing that name be a breaking change? I can't think of a case in which it would be. – Servy Aug 06 '13 at 14:00
  • @Servy Not at all. MS published the code sample on MSDN with `disposing` and it has been used as-is, in production code everywhere, ever since. It's like a regrettable tweet - once it hits the nets, you can never get rid of it! – Keith Payne Aug 06 '13 at 14:16
  • @KeithPayne: If I could change some of that pattern code, I would. I would posit that if a class is going to have a `Disposed` flag, it should be tested and set via `Interlocked` method in the non-virtual `Dispose` handler. There's no other good way to have an overridable thread-safe `Dispose` method. Since an object will have to be created for every time `Dispose` is called (otherwise what would be being disposed?) the overhead of `Interlocked` shouldn't be an issue. – supercat Aug 06 '13 at 14:59
  • @supercat - Thank you for your answer, but it is not quite what I was looking for. I edited the question to hopefully make it more understandable. Sorry about that. Is it okay not to accept the first answer as an answer? – S. Larws Aug 06 '13 at 15:17
  • @S.Larws: If you are designing an abstract class or interface for purpose and factory methods with the abstract class as a return type are sometimes going to return things that have acquired resources and need cleanup, then the abstract class or interface should implement `IDisposable`. If an abstract class, one should probably implement a non-virtual `Dispose` which calls a virtual `Dispose(bool)` that by default does nothing. People writing derived classes will generally expect to add cleanup to a class which already implements `IDisposable` by overriding `Dispose(bool)`. – supercat Aug 06 '13 at 15:22
  • @S.Larws: As an alternative, one could have the abstract class explicitly implement `IDisposable` so it simply does nothing, and figure that derived class authors will notice that `Dispose(bool)` isn't defined and so they need to define it and have `IDisposable.Dispose` call it. If a class is going to have a `Disposed` flag, that should IMHO be stored as an `Int32` and handled using `Interlocked.Exchange` within the non-virtual `Dispose` (a base class which doesn't do that will make it hard to write an inheritable class with a thread-safe `Dispose`), but... – supercat Aug 06 '13 at 15:26
  • ...there's no reason an abstract base class should need to implement a `Disposed` flag if many implementations won't care about disposal. – supercat Aug 06 '13 at 15:26
  • @supercat - I think the expectation is caused by the suggested pattern, but why not split it into a managed and unmanaged part? – S. Larws Aug 06 '13 at 15:32
  • @S.Larws: That would make sense if a majority of derived classes would have resources that need both `Dispose` and `Finalize`, but that isn't the case. If the percentage of derived classes needing `Finalize` is in the 1%-30% range, using one virtual routine for both probably makes more sense. Actually, I think the percentage of derived classes needing `Finalize` for cleanup should be more like 0.0001%; `GC.SuppressFinalize` in the non-virtual Dispose is fine, since it helps with "alarm-bell" finalizers as well as cleanup ones, but the `bool` parameter for `Dispose` isn't really useful. – supercat Aug 06 '13 at 15:42
  • @supercat - I like where this is going. Still all derived classes would themselves have to worry about where the one virtual method was called from. So they would have to do the interpretation of the `bool` parameter once again. Could you please explain why the `bool` parameter is not useful, I don't quite follow :). – S. Larws Aug 06 '13 at 15:48
  • @S.Larws: Nothing should ever call `Dispose(false)`. The only classes that should have finalizers (or destructors) are those whose whole purpose centers around finalization cleanup, or those which use a finalizer not to clean up but instead sound an "object improperly abandoned" alarm. If a class serves some other purpose, its descendants won't fit the first criterion, and those which use finalization for the latter purpose don't need to call `dispose(false)` within it. – supercat Aug 06 '13 at 16:48
  • @supercat - It seems no other answers are coming, so I'll mark your answer as excepted. – S. Larws Aug 07 '13 at 15:27