6

I know that the general guideline for implementing the dispose pattern warns against implementing a virtual Dispose(). However, most often we're only working with managed resources inside a class and so the full dispose pattern seems like an overkill - i.e. we don't need a finalizer. In such cases, is it OK to have a virtual Dispose() in the base class?

Consider this simple example:

abstract class Base : IDisposable
{
    private bool disposed = false;
    public SecureString Password { get; set; } // SecureString implements IDisposable.

    public virtual void Dispose()
    {
        if (disposed)
            return;

        if (Password != null)       
            Password.Dispose();     

        disposed = true;
    }
}

class Sub : Base
{
    private bool disposed = false;
    public NetworkStream NStream { get; set; } // NetworkStream implements IDisposable.

    public override void Dispose()
    {   
        if (disposed)
            return;

        if (NStream != null)
            NStream.Dispose();

        disposed = true;
        base.Dispose();
    }
}

I find this more readable than a complete dispose pattern. I understand that this example would become problematic if we introduced an unmanaged resource into the Base class. But suppose that this won't happen. Is the above example then valid/safe/robust, or does it pose any problem? Should I stick to the standard full-blown dispose pattern, even if no unmanaged resources are used? Or is the above approach indeed perfectly OK in this situation - which in my experience is far more common than dealing with unmanaged resources?

w128
  • 4,680
  • 7
  • 42
  • 65
  • 1
    By "full blown" you mean one extra protected method which is virtual and accepts a boolean? – Yuval Itzchakov Sep 02 '15 at 13:27
  • why your sub also have IDisposable.. may not be required as base has it already – sumeet kumar Sep 02 '15 at 13:37
  • 2
    The dispose pattern is an abomination. In my whole life I have written like 3 finalizers. Push unmanaged handles into handle classes. That way you almost never need a finalizer in your own class. Starting with .NET 2.0 the .NET Framework recognizes that pattern with the SafeHandle class. – usr Sep 02 '15 at 13:49
  • @YuvalItzchakov: yes, [that](https://msdn.microsoft.com/en-us/library/vstudio/b1yfkh5e%28v=vs.100%29.aspx)'s what I mean with "full blown". – w128 Sep 02 '15 at 13:49
  • @sumeetkumar missed that due to writing the sample on the fly, thanks. – w128 Sep 02 '15 at 13:52
  • Explaining the downvotes would be helpful. Is there something obvious rendering this question useless? – w128 Sep 02 '15 at 13:56
  • The "disposed" field is not necessary. You are only disposing managed resources and they should handled correctly whether or not they are already disposed internally - you can safely call dispose on them more than once - if they are already disposed they'll ignor the second call. – Ricibob Sep 02 '15 at 14:22
  • @Ricibob you are technically correct, but the additional bool test can (sometimes significantly) improve Dispose() performance in case it is called multiple times while there are many resources to dispose (i.e. to check for null). – w128 Sep 02 '15 at 14:33
  • @w128 I'd not put that bool field in. `if x != null` is super cheap. The field has costs, too. It clutters the code. It's like 99% of the time not a good thing I think. – usr Sep 02 '15 at 14:47
  • @usr in most cases, yes. However, if you've got a container of e.g. 100000 IDisposable objects and need to iterate them one-by-one and null-test them in Dispose(), then the test is no longer super cheap, especially if someone accidentally calls your Dispose() multiple times (e.g. in a loop). This is easy to test with a stopwatch and the difference can be noticable. Ultimately this choice depends on one's needs and expectations. – w128 Sep 02 '15 at 14:53

2 Answers2

5

I have given up on the full-blown IDisposable pattern in cases where I am not dealing with unmanaged resources.

I don't see any reason why you shouldn't be able to forego the pattern and make Dispose virtual if you can ensure that derived classes won't introduce unmanaged resources!

(And here lies a problem, because you cannot enforce this. You have no control over what fields a derived class will add, so there is no absolute safety with a virtual Dispose. But then even if you used the full-blown pattern, a derived class could get things wrong, so there is always some trust involved that subtypes adhere to certain rules.)

First, in cases where we only deal with managed objects, having a finalizer would be pointless: If Dispose is called from the finalizer (Dispose(disposing: false) according to the full-blown pattern), we may not safely access/dereference other reference-typed managed objects because they might already be gone. Only value-typed objects reachable from the object being finalized may be safely accessed.

If the finalizer is pointless, it is also pointless to have the disposing flag, which is used to distinguish whether Dispose was explicitly called or whether it was called by the finalizer.

It is also unnecessary to do GC.SuppressFinalize if there is no finalizer.

I am not even sure whether it is then still imperative that Dispose not throw any exceptions in a managed-only scenario. AFAIK this rule exists mostly to protect the finalizer thread. (See @usr's comment below.)

As you can see, once the finalizer goes, much of the classic Disposable pattern is no longer necessary.

Community
  • 1
  • 1
stakx - no longer contributing
  • 83,039
  • 20
  • 168
  • 268
  • 1
    "I am not even sure whether it is then still imperative that Dispose not throw any exceptions in a managed-only scenario." That's important so that you can just use `using` and not worry about an exception being replaced by a new Disposed-originated exception. – usr Sep 02 '15 at 13:48
  • @usr: Agreed; I forgot about that for a moment. I've edited my answer accordingly. Thanks for the correction! – stakx - no longer contributing Sep 02 '15 at 13:54
  • Thanks, these were my thoughts exactly. I was afraid I was beginning to experience some cognitive decline because everywhere on the web I looked they promoted the full blown dispose pattern despite that fact that it seems to be very rarely actually needed (even e.g. `SecureString` implementation doesn't use it as it relies on a safe handle). – w128 Sep 02 '15 at 13:55
2

I understand that this example would become problematic if we introduced an unmanaged resource into the Base class. But suppose that this won't happen. Is the above example then valid/safe/robust, or does it pose any problem? Should I stick to the standard full-blown dispose pattern, even if no unmanaged resources are used?

Though your base class uses exclusively managed resources (and this will not change in the future), you cannot guarantee that a derived class will not use unmanaged resources. So consider to implement the Basic Dispose Pattern in your base class (a class with a virtual Dispose(bool) but without a finalizer) even if you always call the Dispose(bool) method with true.

When some day a new subclass will be derived from your Base, which uses unmanaged resources, its finalizer might want to call the Dispose(bool) with false. Of course, you could introduce a new Dispose(bool) in the derived class:

public class SubUnmanaged : Base
{
    IntPtr someNativeHandle;
    IDisposable someManagedResource;

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

    ~SubUnmanaged();
    {
        base.Dispose();
        Dispose(false);
    }

    protected virtual void Dispose(bool disposing)
    {
        if (someNativeHandle != IntPtr.Zero)
            Free(someNativeHandle);

        if (disposing)
           someManagedResource.Dispose();
    }
}

But I think this is a little bit nasty. By sealing the original virtual Dispose() method, you can prevent the further inheritors to be confused because of the multiple virtual Dispose methods, and you can call the common base.Dispose() both from the finalizer and the sealed Dispose(). You must do the same workaround in every subclass, which use unmanaged resources and is derived from Base or a fully managed subclass. But this is already far from the pattern, which always makes the things difficult.

György Kőszeg
  • 17,093
  • 6
  • 37
  • 65
  • The derived class should not have a Finalizer. It should move unmanaged resources into a dedicated class that does not participate in inheritance. I think the pattern shown here show *never* be used. It's OK, but not the best. – usr Sep 02 '15 at 14:48
  • @usr: I generally agree; however, you should not make the base class finalizable if it does not use unmanaged resources. So it is still better to introduce a finalizer in a derived class (not by the nasty workaround I posted in the answer, of course), and simply call `Dispose(false)` from there. See also: https://msdn.microsoft.com/en-us/library/vstudio/b1yfkh5e%28v=vs.110%29.aspx#finalizable_types – György Kőszeg Sep 02 '15 at 15:04
  • Yes, your pattern here is better than the "abomination" that everyone else is using. That's true. – usr Sep 02 '15 at 15:06