Advise needed to complete Dispose/Finalize

Question

Following is a stripped-down version of c# code, which helps to capture the PrintScreen key. This work as I expected and tested.

Question: I am aware of deterministic destruction/disposal pattern, which I started to draft as below. However, I need some expert advice to complete my dispose and finalize method. Any advise ?

public class RegisterPrintKey : IDisposable
    {
        public delegate void HotKeyPass();

        public event HotKeyPass HotKey;

        private IntPtr m_WindowHandle = IntPtr.Zero;

        private MKEY m_ModKey = MKEY.MOD_CONTROL;

        private Keys m_Keys = Keys.A;

        private HotKeyWndProc m_HotKeyWnd = new HotKeyWndProc();


        [DllImport("user32.dll")]
        public static extern bool RegisterHotKey(IntPtr wnd, int id, MKEY mode, Keys vk);

        [DllImport("user32.dll")]
        public static extern bool UnregisterHotKey(IntPtr wnd, int id);

        private class HotKeyWndProc : NativeWindow
        {
            public int m_WParam = 10000;
            public HotKeyPass m_HotKeyPass;

            protected override void WndProc(ref Message m)
            {
                if (m.Msg == 0x0312 && m.WParam.ToInt32() == m_WParam)
                {
                    if (m_HotKeyPass != null) m_HotKeyPass.Invoke();
                }

                base.WndProc(ref m);
            }
        }

        private bool hasDisposed = false;

        protected virtual void Dispose(bool dispose)
        {
            if (hasDisposed) return;

            if (dispose)
            {
                //release objects owned by this instance
                HotKey = null;
                hasDisposed=true;                    


            }
            m_WindowHandle = IntPtr.Zero; // I presume this is not required.
            m_Keys = null; //Do i need to dispose this or relay on stack ( clean up when thread unwind its stack)
            m_ModKey = null;

            m_HotKeyWnd.DestroyHandle();

        }

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

        ~RegisterPrintKey()
        {
            Dispose(false);
        }
    }

    public enum MKEY
        {
        MOD_ALT = 0x0001,
        MOD_CONTROL = 0x0002,
        MOD_SHIFT = 0x0004,
        MOD_WIN = 0x0008,
        }

Answer 1

Some suggestions on your code

public class RegisterPrintKey : IDisposable {
  ...

// This class can allocate the Window Handle resource (HWND)
private class HotKeyWndProc : NativeWindow {
}

// Explicit resource (HWND) allocation
private HotKeyWndProc m_HotKeyWnd = new HotKeyWndProc();

// I'd rather make a property from your "hasDisposed" field:
//   - it's make easier to check instance's state (esp. while debugging)
//   - IsDisposed is more popular name for this
public Boolean IsDisposed {
  get;
  protected set; // <- or even "private set"
}

protected virtual void Dispose(Boolean dispose) {
  if (IsDisposed)
    return;

  if (disposed) {
    // Release any Objects here

    // You've allocated the HWND resource so you have to dispose it: 
    m_HotKeyWnd.DestroyHandle(); // <- Free HWND you've created
  }

  // Here, you may work with structures only!

  // You don't need these assignments, but you can safely do them:
  // mayhaps, they'll be useful for you while debugging 
  m_WindowHandle = IntPtr.Zero; 
  m_Keys = null; 
  m_ModKey = null;

  // Finally, we've done with disposing the instance
  IsDisposed = true; 
}

public void Dispose() {
  Dispose(true);
  // We've done with Dispose: "GC, please, don't bother the disposed instance"
  GC.SuppressFinalize(this);
}

// A treacherous enemy I've commented out: 
// if you've made a error in Dispose() it'll be resource leak 
// or something like AccessViolation. The only good thing is that
// you can easily reproduce (and fix) the problem.
// If you've uncommented ~RegisterPrintKey() this leak/EAV will become
// floating error: it'll appear and disappear on different workstations
// OSs etc: you can't control GC when to run. Floating error is
// much harder to detect.

// Another bad issue with finalizer is that it prevents the instance 
// from being in zero generation, see
// http://stackoverflow.com/questions/12991692/why-doesnt-object-with-finalizer-get-collected-even-if-it-is-unrooted

//~RegisterPrintKey() {
//   // This code can be called (if called!) at random time
//   Dispose(false); // <- That's an enemy! 
// }
}

Answer 2

The basic idea is not to touch anything managed in the Dispose(false) variant of your method. Also, there's usually no need to explicitly set anything to null (especially in the Dispose(false) variant, where those objects are probably already garbage collected).

So, you've got the basic pattern right, but you don't need to do anything in Dispose(false) except for the m_HotKeyWnd.DestroyHandle().

To explain a bit more, when the finalizer code is run by the garbage collector, all of the other managed objects referenced by this object (that aren't referenced by others) are probably garbage collected already. Ignore everything that's managed in the finalizer, it probably doesn't even exist anymore. It's also something you can expect in the native callback (ie. your WndProc in this case) - it's very much possible that the managed objects you interact with don't exist anymore, if the callback comes while the object is on the finalizer queue. This is a fairly common cause of application crashes (or at least of unexpected exceptions) in a managed application interacting with native code.

So, Dispose(true) should handle everything you want to clean up, managed and unmanaged (and you're correctly using GC.SuppressFinalize - you've already disposed of the unmanaged resources, so there's no need for GC to put your object on the finalizer queue), Dispose(false) should only ever handle the unmanaged bits.

EDIT: I didn't realize that your m_HotKeyWnd is actually a managed object - it takes care of its unmanaged resources on its own, and you should in no case call its DestroyHandle from the finalizer. In fact, you don't even need the finalizer, its completely redundant (and thus, slightly harmful). Just implement a simple Dispose (instead of the usual finalizer-dispose pattern) that disposes of the m_HotKeyWnd and sets it to null (NullReferenceException is better in this case than AccessViolationException or undefined behaviour you could get when using the object after its been disposed of - unmanaged stuff gets tricky fast), and does nothing else.