Dispose pattern - DisposableObject

Question

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 already disposed." + GetTypeString());
 }
 if (IsDisposing)
 {
 throw new ObjectDisposedException("Object is being disposed." + GetTypeString());
 }
 }
 private void Dispose(bool disposeManagedResources)
 {
 if (IsDisposed)
 {
 throw new InvalidOperationException("Dispose called on an object that is already disposed." + GetTypeString());
 }
 if (IsDisposing)
 {
 throw new InvalidOperationException("Dispose called on an object that is currently disposing." + GetTypeString());
 }
 IsDisposing = true;
 if (disposeManagedResources)
 {
 DisposeManagedResources();
 VerifyBaseDisposeManagedResourcesCalled();
 }
 if (hasUnmanagedResources)
 {
 DisposeUnmanagedResources();
 VerifyBaseDisposeUnmanagedResourcesCalled();
 }
 IsDisposed = true;
 IsDisposing = false;
 }
 private void VerifyBaseDisposeManagedResourcesCalled()
 {
 if (!baseDisposeManagedResourcesCalled)
 {
 throw new InvalidOperationException("DisposeManagedResources of the base class was not called." + GetTypeString());
 }
 }
 private void VerifyBaseDisposeUnmanagedResourcesCalled()
 {
 if (!baseDisposeUnmanagedResourcesCalled)
 {
 throw new InvalidOperationException("DisposeUnmanagedResources of the base class was not called." + GetTypeString());
 }
 }
 private string GetTypeString()
 {
 string typeName = GetType().FullName;
 return " Type: " + typeName;
 }
}

By default, the object is removed from the finalization queue (in the DisposableObject constructor). If some inheriting class has an unmanaged resource then it should invoke HasUnmanagedResources method to add the object back to the finalization queue.

The most important question is: can I invoke GC.SuppressFinalize(this) in the constructor?.

How do you find this implementation?

• Damien_The_Unbeliever: You only get to pick one base class - I'd have thought it unlikely that gaining a good Disposable implementation would be the most pressing requirement for most interesting classes.
• Tom Chantler: But surely the interesting base class could itself inherit from Disposable. And if not, keep working your way back until it can... :-)
• Alessandro D'Andria: Why don't follow the standard Dispose pattern and move the GC.SuppressFinalize(this); to the finalizer?
• Alessandro D'Andria: I have used a similar aproach in the past and really it gave me no problems, but I had always a mix feeling about it, one issue I faced was that this it's useless with classes that must override from base out of your control, and for unmanaged resources SafeHandle is always the best choice to handle them.
• Henk Holterman: It's a pattern for a reason. If a base-class implementation was optimal we would have it. This looks overly complicated and should be mostly unnecessary because of SafeHandle.

Answer 1

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:

1. Constructors should be protected because the base class is abstract. This would make the code clearer to me, but YMMV.
2. Lose the whole unmanaged resource tracking. It adds complexity without value. Just call SuppressFinalize() regardless - it's the recommended pattern, anyway.
3. 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;
 /// <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 has been disposed or is in the process of being disposed.
 /// </summary>
 protected bool IsDisposedOrDisposing
 {
 get { return Interlocked.CompareExchange(ref this.disposeStage, DisposalNotStarted, DisposalNotStarted) != DisposalNotStarted; }
 }
 /// <summary>
 /// Gets the object name, for use in any <see cref="ObjectDisposedException"/> thrown by this object.
 /// </summary>
 /// <remarks>
 /// Subclasses can override this property if they would like more control over the object name appearing in any <see cref="ObjectDisposedException"/>
 /// thrown by this <c>DisposableBase</c>. This can be particularly useful in debugging and diagnostic scenarios.
 /// </remarks>
 /// <returns>
 /// The object name, which defaults to the class name.
 /// </returns>
 protected virtual string ObjectName
 {
 get { return this.GetType().FullName; }
 }
 /// <summary>
 /// Disposes of this object, if it hasn't already been disposed.
 /// </summary>
 [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.disposeStage, DisposalStarted, DisposalNotStarted) != DisposalNotStarted)
 {
 return;
 }
 this.OnDisposing();
 this.Disposing = null;
 this.Dispose(true);
 this.MarkAsDisposed();
 }
 /// <summary>
 /// Verifies that this object is not in the process of disposing, throwing an exception if it is.
 /// </summary>
 protected void VerifyNotDisposing()
 {
 if (this.IsDisposing)
 {
 throw new ObjectDisposedException(this.ObjectName);
 }
 }
 /// <summary>
 /// Verifies that this object has not been disposed, throwing an exception if it is.
 /// </summary>
 protected void VerifyNotDisposed()
 {
 if (this.IsDisposed)
 {
 throw new ObjectDisposedException(this.ObjectName);
 }
 }
 /// <summary>
 /// Verifies that this object is not being disposed or has been disposed, throwing an exception if either of these are true.
 /// </summary>
 protected void VerifyNotDisposedOrDisposing()
 {
 if (this.IsDisposedOrDisposing)
 {
 throw new ObjectDisposedException(this.ObjectName);
 }
 }
 /// <summary>
 /// Allows subclasses to provide dispose logic.
 /// </summary>
 /// <param name="disposing">
 /// Whether the method is being called in response to disposal, or finalization.
 /// </param>
 protected virtual void Dispose(bool disposing)
 {
 }
 /// <summary>
 /// Raises the <see cref="Disposing"/> event.
 /// </summary>
 protected virtual void OnDisposing()
 {
 this.Disposing.Raise(this);
 }
 /// <summary>
 /// Marks this object as disposed without running any other dispose logic.
 /// </summary>
 /// <remarks>
 /// Use this method with caution. It is helpful when you have an object that can be disposed in multiple fashions, such as through a <c>CloseAsync</c> method.
 /// </remarks>
 [SuppressMessage("Microsoft.Usage", "CA1816", Justification = "This is a helper method for IDisposable.Dispose.")]
 protected void MarkAsDisposed()
 {
 GC.SuppressFinalize(this);
 Interlocked.Exchange(ref this.disposeStage, DisposalComplete);
 }
 }
}
#endregion Designer generated code
#pragma warning restore 1591, 0612

• Mathieu Guindon: Well that's an interesting approach!
• Pellared: I love your solution! Thanks :) One question: Why are you deriving from object? Lose the whole unmanaged resource tracking. It adds complexity without value. There is a value - it is about the finalization queue. When an object is in the queue than it can enter easily into next generation (Gen1 or even in some cases Gen2). Quote from MSDN: X AVOID making types finalizable. Carefully consider any case in which you think a finalizer is needed. There is a real cost associated with instances with finalizers, from both a performance [...]
• Pellared: All your object that are generated using this pattern will enter the finiazlization queue.
• Kent Boogaart: @Pellared: only in DEBUG builds will these objects potentially enter the finalization queue (and that's just to add a useful assertion - remove it if you prefer). If they're disposed proactively, they don't even get to the finalization queue because SuppressFinalize is called. As for deriving from object, the base class comes from a t4 template parameter. So it can derive whatever you tell it to derive from - I just chose the simplest case: deriving from object. I have several others, such as deriving from ReactiveObject.
• Pellared: @KentBoogaart After some experience I must say that your implementation is really awesome. I also like the implementation of Disposable in AutoFac: github.com/autofac/Autofac/blob/master/Core/Source/Autofac/Util/… simple but robust!

Answer 2

This abstraction is a leaky abstraction and should be avoided.

As Mark Seeman puts it:

Consider why you want to add IDisposable to your interface. It's probably because you have a particular implementation in mind. Hence, the implementation leaks into the abstraction.

Now in this specific context he's talking about ISomeInterface : IDisposable, but the same applies to an abstract class, because an abstract class is an abstraction just like an interface is.

Implementing it in a base class prevents any derived class from deriving from anything else, because in C# a class can only derive from a single base class. This reason alone is a showstopper.

Also, calling Dispose() twelve times in a row on a disposable object should not throw anything. Only using a disposed object should; your implementation breaks the pattern in ways that break POLS.

• Pellared: In my opnion your comment would be true for interfaces, domain models, decorators etc. However here we have an abstraction of implementation to minimize code redundation in simple scenarios when we do not need to derive anything. Honestly I would prefer doing such thing as an Aspect (AoP) but no idea how it could be done...
• Mathieu Guindon: AOP buys you vendor lock-in (e.g. PostSharp) and boils down to a hack that injects code into the MSIL generated when you build your project. I think it's utterly bad and would highly recommend implementing aspects with IoC and interception instead.
• Mathieu Guindon: If you have an abstraction of implementation to minimize code redundation in simple scenarios then you have the wrong reasons for inheritance.
• Pellared: AOP using IoC interception is really slow if we are resolving many objects (tested by my friend). As I think of the usage of the DisposableObject in the project... Honestly I start feeling that you are right that this implementation should not be ever used... Thanks!
• Mathieu Guindon: Inheritance is for "is-a" relationships. Functionality encapsulated in a ViewModelBase class is meant to be used by classes that are ViewModels. Now I foresee you'll argue that a class derived from DisposableObject is-a disposable object, but I still think it's a leaky abstraction (didn't say I never leak abstractions, just that it should be avoided).