Gimly
Gimly

Reputation: 6175

Why does the recommended dispose pattern adds a disposed field at each level of the hierarchy?

The Dispose pattern is notoriously complicated to get right, especially when we have a hierarchy of classes that needs to dispose things at different levels. The recommended implementation is as follow, took from Implement a Dispose method - Microsoft Docs.

using System;

class BaseClass : IDisposable
{
    // To detect redundant calls
    private bool _disposed = false;

    ~BaseClass() => Dispose(false);

    // Public implementation of Dispose pattern callable by consumers.
    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    // Protected implementation of Dispose pattern.
    protected virtual void Dispose(bool disposing)
    {
        if (_disposed)
        {
            return;
        }

        if (disposing)
        {
            // TODO: dispose managed state (managed objects).
        }

        // TODO: free unmanaged resources (unmanaged objects) and override a finalizer below.
        // TODO: set large fields to null.

        _disposed = true;
    }
}

using Microsoft.Win32.SafeHandles;
using System;
using System.Runtime.InteropServices;

class DerivedClass : BaseClass
{
    // To detect redundant calls
    private bool _disposed = false;

    // Instantiate a SafeHandle instance.
    private SafeHandle _safeHandle = new SafeFileHandle(IntPtr.Zero, true);

    // Protected implementation of Dispose pattern.
    protected override void Dispose(bool disposing)
    {
        if (_disposed)
        {
            return;
        }

        if (disposing)
        {
           // Dispose managed state (managed objects).
            _safeHandle?.Dispose();
        }

        _disposed = true;

        // Call base class implementation.
        base.Dispose(disposing);
    }
}

What I don't get in this implementation is what advantage we have in adding a _disposed field at each level of the hierarchy? Instead, we could take care of the _disposed only in the top level of the hierarchy (the one implementing directly IDisposable and not care about it in the derived classes.

Something like this:

using System;

class BaseClass : IDisposable
{
    // To detect redundant calls
    private bool _disposed = false;

    ~BaseClass()
    {
       if (_disposed)
       {
          return;
       }

       Dispose(false);
       _disposed = true;
    }

    // Public implementation of Dispose pattern callable by consumers.
    public void Dispose()
    {
        if (_disposed)
        {
            return;
        }

        Dispose(true);
        GC.SuppressFinalize(this);
        _disposed = true;
    }

    // Protected implementation of Dispose pattern.
    protected virtual void Dispose(bool disposing)
    {
        if (disposing)
        {
            // TODO: dispose managed state (managed objects).
        }

        // TODO: free unmanaged resources (unmanaged objects) and override a finalizer below.
        // TODO: set large fields to null.
    }
}

using Microsoft.Win32.SafeHandles;
using System;
using System.Runtime.InteropServices;

class DerivedClass : BaseClass
{
    // Instantiate a SafeHandle instance.
    private SafeHandle _safeHandle = new SafeFileHandle(IntPtr.Zero, true);

    // Protected implementation of Dispose pattern.
    protected override void Dispose(bool disposing)
    {
        if (disposing)
        {
           // Dispose managed state (managed objects).
            _safeHandle?.Dispose();
        }

        // Call base class implementation.
        base.Dispose(disposing);
    }
}

This is such a widely used code sample that there must certainly be a good reason for it to be implemented with the repeated _disposed at each level, but I just can't find any.

It's a tiny bit more code in the base class, but less to worry about in the derived class, and some repeated information removed.

What else am I missing?

Edit:

As @InBetween correctly says that one drawback of my implementation is that if you'd need to check if your object is disposed in one method of the derived class you won't be able to check it. Let's correct that issue by making it a protected property with a private set.

using System;

class BaseClass : IDisposable
{
    // To detect redundant calls
    protected bool Disposed { get; private set; } = false;

    ~BaseClass()
    {
       if (Disposed)
       {
          return;
       }

       Dispose(false);
       Disposed = true;
    }

    // Public implementation of Dispose pattern callable by consumers.
    public void Dispose()
    {
        if (Disposed)
        {
            return;
        }

        Dispose(true);
        GC.SuppressFinalize(this);
        Disposed = true;
    }

    // Protected implementation of Dispose pattern.
    protected virtual void Dispose(bool disposing)
    {
        if (disposing)
        {
            // TODO: dispose managed state (managed objects).
        }

        // TODO: free unmanaged resources (unmanaged objects) and override a finalizer below.
        // TODO: set large fields to null.
    }
}

using Microsoft.Win32.SafeHandles;
using System;
using System.Runtime.InteropServices;

class DerivedClass : BaseClass
{
    // Instantiate a SafeHandle instance.
    private SafeHandle _safeHandle = new SafeFileHandle(IntPtr.Zero, true);

    public void DoSomething()
    {
       if(Disposed)
       {
           throw new ObjectDisposedException("Cannot access disposed resource");
       }
    }    

    // Protected implementation of Dispose pattern.
    protected override void Dispose(bool disposing)
    {
        if (disposing)
        {
           // Dispose managed state (managed objects).
            _safeHandle?.Dispose();
        }

        // Call base class implementation.
        base.Dispose(disposing);
    }
}

Upvotes: 4

Views: 625

Answers (2)

John Wu
John Wu

Reputation: 52290

If you are inheriting from a disposable class, one of two conditions must be true.

  1. Your subcless does not introduce a new disposable resource. In this case you don't need to override Dispose and the question is moot.

  2. Your subclass introduces a new disposable resource. In this case, you're going to override Dispose, insert your own disposal code, then call base.Dispose. The _disposed flag is there to help you remember to prevent your disposal code from executing twice.

You certainly can remove _disposed if you want. But you probably do not care much about the base class' _disposed flag, if it even has one. It worries about what it is disposing, and you worry about yours.

Upvotes: 4

InBetween
InBetween

Reputation: 32780

The reason is simple. In your implementation you cant use _disposed in the derived type to check if any method is invoked when the object is already disposed and take the necessary actions. In your implementation you'd need to create your own redundant flag isDisposed which defeats the purpose; you already have one "for free" from the pattern itself following the guidelines.

A case could be made though about making _disposed protected.

Upvotes: 3

Related Questions