HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Dispose pattern - DisposableObject

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
disposableobjectpatterndispose

Problem

I am trying to make an universal implementation of IDisposable (as a base class):

```
public abstract class DisposableObject : IDisposable
{
private bool hasUnmanagedResources;
private bool baseDisposeManagedResourcesCalled;
private bool baseDisposeUnmanagedResourcesCalled;

protected bool IsDisposed { get; private set; }
protected bool IsDisposing { get; private set; }

public DisposableObject()
: this(false)
{
}

public DisposableObject(bool hasUnmanagedResources)
{
this.hasUnmanagedResources = hasUnmanagedResources;

if (!hasUnmanagedResources)
{
GC.SuppressFinalize(this);
}
}

~DisposableObject()
{
Dispose(false);
}

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

protected virtual void DisposeManagedResources()
{
if (!IsDisposing)
{
throw new InvalidOperationException(
"In order to dispose an object call the Dispose method. Do not call DisposeManagedResources method directly." +
GetTypeString());
}

baseDisposeManagedResourcesCalled = true;
}

protected virtual void DisposeUnmanagedResources()
{
if (!IsDisposing)
{
throw new InvalidOperationException(
"In order to dispose an object call the Dispose method. Do not call DisposeUnmanagedResources method directly." +
GetTypeString());
}

baseDisposeUnmanagedResourcesCalled = true;
}

protected void HasUnmanagedResources()
{
if (!hasUnmanagedResources)
{
hasUnmanagedResources = true;
GC.ReRegisterForFinalize(this);
}
}

protected void ThrowIfDisposed()
{
if (IsDisposed)
{
throw new ObjectDisposedException("Object is al

Solution

Firstly, to address inheritance concerns, I use a t4 template to get around the single inheritance problem. ie. for every class where I need to introduce dispose support, I generate the an abstract base class that inherits from that class. Works a treat.

Now, to your code. Here are the things that jump out at me:

  • Constructors should be protected because the base class is abstract. This would make the code clearer to me, but YMMV.



  • Lose the whole unmanaged resource tracking. It adds complexity without value. Just call SuppressFinalize() regardless - it's the recommended pattern, anyway.



  • Your code is not thread-safe. Consider, for example, what happens if multiple threads call Dispose() simultaneously. You're got the potential for multiple calls to DisposeManagedResources() etcetera.



FWIW, here is an example Disposable base class generated by my t4 template:

```
// Generated by t4 template in Utility project. DO NOT EDIT!
#pragma warning disable 1591, 0612
#region Designer generated code

namespace Foo
{
using System;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Globalization;
using System.Threading;
using Kent.Boogaart.HelperTrinity.Extensions;

///
/// A useful base class for disposable objects that inherit from .
///
///
///
/// This base class simplifies the implementation of disposable objects by providing common functionality such as thread-safety on calls,
/// and a event for pre-disposal notification.
///
///
public abstract class DisposableBase : object, IDisposable
{
private const int DisposalNotStarted = 0;
private const int DisposalStarted = 1;
private const int DisposalComplete = 2;

#if DEBUG
// very useful diagnostics when a failure to dispose is detected
private readonly StackTrace creationStackTrace;
#endif

// see the constants defined above for valid values
private int disposeStage;

#if DEBUG
///
/// Initializes a new instance of the DisposableBase class.
///
protected DisposableBase()
{
this.creationStackTrace = new StackTrace(1, true);
}

///
/// Finalizes an instance of the DisposableBase class.
///
[SuppressMessage("Microsoft.Design", "CA1063", Justification = "The enforced behavior of CA1063 is not thread-safe or full-featured enough for our purposes here.")]
~DisposableBase()
{
var message = string.Format(CultureInfo.InvariantCulture, "Failed to proactively dispose of object, so it is being finalized: {0}.{1}Object creation stack trace:{1}{2}", this.ObjectName, Environment.NewLine, this.creationStackTrace);
Debug.Assert(false, message);
this.Dispose(false);
}
#endif

///
/// Occurs when this object is about to be disposed.
///
public event EventHandler Disposing;

///
/// Gets a value indicating whether this object is in the process of disposing.
///
protected bool IsDisposing
{
get { return Interlocked.CompareExchange(ref this.disposeStage, DisposalStarted, DisposalStarted) == DisposalStarted; }
}

///
/// Gets a value indicating whether this object has been disposed.
///
protected bool IsDisposed
{
get { return Interlocked.CompareExchange(ref this.disposeStage, DisposalComplete, DisposalComplete) == DisposalComplete; }
}

///
/// Gets a value indicating whether this object has been disposed or is in the process of being disposed.
///
protected bool IsDisposedOrDisposing
{
get { return Interlocked.CompareExchange(ref this.disposeStage, DisposalNotStarted, DisposalNotStarted) != DisposalNotStarted; }
}

///
/// Gets the object name, for use in any thrown by this object.
///
///
/// Subclasses can override this property if they would like more control over the object name appearing in any
/// thrown by this DisposableBase. This can be particularly useful in debugging and diagnostic scenarios.
///
///
/// The object name, which defaults to the class name.
///
protected virtual string ObjectName
{
get { return this.GetType().FullName; }
}

///
/// Disposes of this object, if it hasn't already been disposed.
///
[SuppressMessage("Microsoft.Design", "CA1063", Justification = "The enforced behavior of CA1063 is not thread-safe or full-featured enough for our purposes here.")]
[SuppressMessage("Microsoft.Usage", "CA1816", Justification = "GC.SuppressFinalize is called indirectly.")]
public void Dispose()
{
if (Interlocked.CompareExchange(ref this.

Code Snippets

// Generated by t4 template in Utility project. DO NOT EDIT!
#pragma warning disable 1591, 0612
#region Designer generated code

namespace Foo
{
    using System;
    using System.Diagnostics;
    using System.Diagnostics.CodeAnalysis;
    using System.Globalization;
    using System.Threading;
    using Kent.Boogaart.HelperTrinity.Extensions;

    /// <summary>
    /// A useful base class for disposable objects that inherit from <see cref="object"/>.
    /// </summary>
    /// <remarks>
    /// <para>
    /// This base class simplifies the implementation of disposable objects by providing common functionality such as thread-safety on <see cref="Dispose"/> calls,
    /// and a <see cref="Disposing"/> event for pre-disposal notification.
    /// </para>
    /// </remarks>
    public abstract class DisposableBase : object, IDisposable
    {
        private const int DisposalNotStarted = 0;
        private const int DisposalStarted = 1;
        private const int DisposalComplete = 2;

#if DEBUG
        // very useful diagnostics when a failure to dispose is detected
        private readonly StackTrace creationStackTrace;
#endif

        // see the constants defined above for valid values
        private int disposeStage;

#if DEBUG
        /// <summary>
        /// Initializes a new instance of the DisposableBase class.
        /// </summary>
        protected DisposableBase()
        {
            this.creationStackTrace = new StackTrace(1, true);
        }

        /// <summary>
        /// Finalizes an instance of the DisposableBase class.
        /// </summary>
        [SuppressMessage("Microsoft.Design", "CA1063", Justification = "The enforced behavior of CA1063 is not thread-safe or full-featured enough for our purposes here.")]
        ~DisposableBase()
        {
            var message = string.Format(CultureInfo.InvariantCulture, "Failed to proactively dispose of object, so it is being finalized: {0}.{1}Object creation stack trace:{1}{2}", this.ObjectName, Environment.NewLine, this.creationStackTrace);
            Debug.Assert(false, message);
            this.Dispose(false);
        }
#endif

        /// <summary>
        /// Occurs when this object is about to be disposed.
        /// </summary>
        public event EventHandler Disposing;

        /// <summary>
        /// Gets a value indicating whether this object is in the process of disposing.
        /// </summary>
        protected bool IsDisposing
        {
            get { return Interlocked.CompareExchange(ref this.disposeStage, DisposalStarted, DisposalStarted) == DisposalStarted; }
        }

        /// <summary>
        /// Gets a value indicating whether this object has been disposed.
        /// </summary>
        protected bool IsDisposed
        {
            get { return Interlocked.CompareExchange(ref this.disposeStage, DisposalComplete, DisposalComplete) == DisposalComplete; }
        }

        /// <summary>
        /// Gets a value indicating whether this object ha

Context

StackExchange Code Review Q#32380, answer score: 9

Revisions (0)

No revisions yet.