Is my IDisposable implemented correctly in my class or needs some changes

Question

I have got below code in C# 2.0 and I am trying to implement IDisposable in my class.

using System;
using System.Collections.Generic;
using System.Text;
using System.Text.RegularExpressions;
using System.Xml;
using System.Xml.XPath;
using System.Xml.Xsl;
using Tridion.ContentManager;
using Tridion.ContentManager.Templating.Assembly;
using Tridion.ContentManager.Templating;
using Tridion.ContentManager.ContentManagement;
using Tridion.ContentManager.ContentManagement.Fields;
using Emirates.Tridion.BuildingBlocks.Base;
using th = my.Tridion.BuildingBlocks.Base.TemplateHelper;
using ut = my.Tridion.BuildingBlocks.Base.Utilities;
using tc = Tridion.ContentManager.CommunicationManagement;
using System.IO;
using System.Globalization;
using System.Threading;

namespace my.BuildingBlocks.Utilities
{

    [TcmTemplateTitle("Page Metadata Values")]
    public class PageMetaDataValues : TemplateBase, IDisposable
    {

        private bool m_Disposed = false;

        protected bool Disposed
        {
            get
            {
                lock (this)
                {
                    return (m_Disposed);
                }
            }
        }


        public override void Transform(Engine engine, Package package)
        {
            Initialize(engine, package);
            m_Logger.Info("Start of Page Metadata Values");
            tc.Publication pubObject= m_Engine.GetSession().GetObject(m_Publication.Id) as tc.Publication;          
            if (pubObject != null)
            {
                Thread.CurrentThread.CurrentCulture = new CultureInfo(ut.RenderPageLocale(pubObject));
            }
            package.PushItem("PageMetaDataValues", package.CreateStringItem(ContentType.Xml, RenderCurrentPageXML()));
            m_Logger.Info("End of Page Metadata Values");
        }

        private string RenderCurrentPageXML()
        {
            m_Logger.Info("Rendering the Page Metadata Values");

            XmlDocument pageDoc = new XmlDocument();
            pageDoc.LoadXml(GetCurrentPageXML(m_Page.Id));
            return pageDoc.InnerXml;
        }      

        void IDisposable.Dispose
        {   
            get
            {
                lock (this)
                {
                    if (m_Disposed == false)
                    {
                        Cleanup();
                        m_Disposed = true;
                        GC.SuppressFinalize(this);
                    }
                }
            }

        }
        protected virtual void Cleanup()
        {
            /* do cleanup of unmanaged resources here */
        }
        #endregion

    }
}   

Please suggest whether I have implemnted IDisposable interface correclty or I need to do some code changes above.

Answer 1

As written by others (the ControlPower solution is correct), the Dispose method is a method, not a property :-).

Now we will look at another problem: you are trying to implement a thread safe IDisposable pattern. To do it correctly is quite complex. Let's see the naive solution. Yours (with the corrections of Controlpower) is ok :-) Clearly you should check the Disposed property at the beginning of each public method/property and if it's true, you should throw an ObjectDisposedException. A better (faster) implementation is the one given by http://blogs.msdn.com/b/blambert/archive/2009/07/24/a-simple-and-totally-thread-safe-implementation-of-idisposable.aspx

he uses Interlocked.CompareExchange instead of lock. It's faster.

It still suffers from one problem: if Thread A executes method A1 and at the same time Thread B executes a Dispose of the object, Thread A could be mid-method when the object is disposed. I'll give you a third solution. It used ReaderWriterLockSlim. Every public method/property takes a reader lock when it executes. Dispose takes a write lock when it executes. Many "normal" methods can be executed at the same time. Dispose needs to be executed when no one else is executing. Private and protected methods/properties (that aren't part of an interface) don't need to check for the disposed state because they are called from public methods/properties.

class MyClass : IDisposable
{
    protected ReaderWriterLockSlim disposeLock = new ReaderWriterLockSlim(LockRecursionPolicy.SupportsRecursion);
    protected bool isDisposed = false; 

    ~MyClass()
    {
        this.Dispose(false);
    }

    public int ExamplePublicProperty
    {
        get
        {
            disposeLock.EnterReadLock();

            try
            {
                if (isDisposed)
                {
                    throw new ObjectDisposedException(GetType().FullName);
                }

                // The property

                return 0; // What you want to return
            }
            finally
            {
                disposeLock.ExitReadLock();
            }
        }

        set
        {
            disposeLock.EnterReadLock();

            try
            {
                if (isDisposed)
                {
                    throw new ObjectDisposedException(GetType().FullName);
                }

                // The property
            }
            finally
            {
                disposeLock.ExitReadLock();
            }
        }
    }

    // The same for private methods
    protected int ExampleProtectedProperty
    {
        // Here we don't need to check for isDisposed
        get;
        set;
    }

    public void ExamplePublicMethod()
    {
        disposeLock.EnterReadLock();

        try
        {
            if (isDisposed)
            {
                throw new ObjectDisposedException(GetType().FullName);
            }

            // The method
        }
        finally
        {
            disposeLock.ExitReadLock();
        }
    }

    // The same for private methods
    protected void ExampleProtectedMethod()
    {
        // Here we don't need to check for isDisposed
    }

    #region IDisposable Members

    public void Dispose()
    {
        disposeLock.EnterWriteLock();

        try
        {
            if (isDisposed)
            {
                return;
            }

            Dispose(true);
            GC.SuppressFinalize(this);

            isDisposed = true;
        }
        finally
        {
            disposeLock.ExitWriteLock();
        }
    }

    #endregion

    protected virtual void Dispose(bool disposing)
    {
        // do the freeing
    }
}

Answer 2

That's not the normal IDisposable pattern, typically you'd do something more like:

public class DisposableClass : IDisposable
{
    ~DisposableClass()
    {
        Dispose(false);
    }

    private bool disposed = false;
    protected bool Disposed
    {
        get
        {
            return (disposed);
        }
    }

    public override void Transform(Engine engine, Package package)
    {
        if( Disposed )
        {
             throw new ObjectDisposedException();
        }

        Initialize(engine, package);
        m_Logger.Info("Start of Page Metadata Values");
        tc.Publication pubObject= m_Engine.GetSession().GetObject(m_Publication.Id) as tc.Publication;          
        if (pubObject != null)
        {
            Thread.CurrentThread.CurrentCulture = new CultureInfo(ut.RenderPageLocale(pubObject));
        }
        package.PushItem("PageMetaDataValues", package.CreateStringItem(ContentType.Xml, RenderCurrentPageXML()));
        m_Logger.Info("End of Page Metadata Values");
    }

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

    protected virtual void Dispose(bool disposing)
    {
        if (disposing)
        {
            disposed = true;
            // Clean up all managed resources
        }

        // Clean up all native resources
    }
}

Answer 3

Just change

    void IDisposable.Dispose
    {   
        get
        {
            lock (this)
            {
                if (m_Disposed == false)
                {
                    Cleanup();
                    m_Disposed = true;
                    GC.SuppressFinalize(this);
                }
            }
        }

    }

to

    public void IDisposable.Dispose()
    {   
            lock (this)
            {
                if (m_Disposed == false)
                {
                    Cleanup();
                    m_Disposed = true;
                    GC.SuppressFinalize(this);
                }
            }
    }

Answer 4

You should add a finializer that calls Cleanup to. In case someone don't call Dispose as they should, the unmanaged resources will then eventually be cleaned up by the garbage collector.

That's why you are calling GC.SuppressFinalize(this); in the first place, to make sure that the GC does not call your finalizer if the object was disposed properly.

EDIT

That means that you at least need to add:

public ~PageMetaDataValues(){
  Cleanup();
}

There are however some other strange cases in your code like locking on this and so on. Also, are you expecting to dispose the same object from several different threads since you added the locking in the first place?