Should IDisposable::Dispose() be virtual

Question

Say a factory for SomeDisposable actually is creating/returning a sort of watch dog Wrapper

public class Wrapper : SomeDisposable
{
    public new /*:(*/ Dispose() { ... };
}

and the caller uses like

using (SomeDisposable sd = SomeDisposableFactory.Create(...))
{
} // Wrapper.Dispose() never called.

The Wrapper.Dispose() is never called. If Dispose() were virtual then Wrapper.Dispose() would be called.

The IDisposable interface does not guarantee that the other best practice method virtual Dispose(bool) actually exists or enforce that either be virtual so it cannot be generally relied on to exist (it is only a recommended pattern). Interfaces currently do not allow constraints on virtual.

What are some pros and cons for not making the recommended Dispose() pattern virtual which would have solved this particular dilemma. Should C# allow a way of forcing virtual methods via an interface (since abstract classes aren't popular as contract definitions).

Found a [duplicate](http://stackoverflow.com/questions/3619490/why-should-dispose-be-non-virtual) — crokusek, Mar 04 '14 at 21:01

score 4 · Accepted Answer · answered Mar 04 '14 at 20:36

No. The pattern actually says that Dispose() (non-virtual) should call a protected virtual void Dispose(bool) method. This guarantees that the base class Dispose call can pass up the hierarchy properly.

This is spelled out in the documentation for IDisposable:

It should provide one public, non-virtual Dispose() method and a protected virtual Dispose(Boolean disposing) method.
The Dispose() method must call Dispose(true) and should suppress finalization for performance.
The base type should not include any finalizers.

Accepting this answer while wishing the "should's" and "must's" from the docs were specified/enforceable by C# itself. — crokusek, Mar 04 '14 at 22:48

Cory Nelson · Answer 2 · 2014-03-04T20:51:51.947

0

This is already solved.

Disposable types which are not sealed should use the common dispose pattern:

public class DisposableResourceHolder : IDisposable
{
    private SafeHandle resource; // handle to a resource

    public DisposableResourceHolder()
    {
        this.resource = ... // allocates the resource
    }

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

    protected virtual void Dispose(bool disposing)
    {
        if (disposing)
        {
            // dispose managed resources.
            if (resource != null) resource.Dispose();
        }

        // free unmanaged resources.
    }
}

edited Mar 04 '14 at 20:51

answered Mar 04 '14 at 20:37

Cory Nelson

29,236
5
72
110

2

I see this a lot. This is encouraging people to use finalizers where they are not necessarily needed. You should explain that the finalizer should only be used if the class has unmanaged resources. When you add a finalizer, you automatically make the object a long lived object and it will always be elevated to at least a level 1 generation object in the garbage collector. – stevethethread Mar 04 '14 at 20:44
@stevethethread excellent point. A derived class can always define their own finalizer if they have the need. – Cory Nelson Mar 04 '14 at 20:52
1

@stevethethread your point may be valid, but why would i want to implement `IDisposable` if i don't have unmanaged resources? – oɔɯǝɹ Mar 04 '14 at 21:16
Managed resources. E.g. Database connections etc. The point of a finalizer is to ensure unmanged resources are cleaned up if dispose has not be called. Hence why you call GC.SuppressFinalize in your dispose method, as it means Dispose has been called and resource have been cleaned up. – stevethethread Mar 04 '14 at 21:27
@stevethethread: The call to `GC.SuppressFinalize()` probably isn't really needed, since the only classes which should define cleanup finalizers are those which derive directly from `System.Object`, but in some cases it may be necessary to ensure that the call to `Dispose` can't get optimized out; `GC.KeepAlive(this)` would be a fine way to do that, but `GC.SuppressFinalize(this)` is just as good. – supercat Mar 06 '14 at 01:12

Should IDisposable::Dispose() be virtual

2 Answers2