Wrapping COM objects with IDisposable

Question

One of the things believed to contribute to destabilizing Rubberduck 2.x, is the fact that a lot of COM object references are stored in many places, and Marshal.ReleaseComObject is never called for those, which is what the alleged problem would be.

To fix this, I've undertaken the task of completely wrapping the VBIDE API with managed types that implement IDisposable and call Marshal.ReleaseComObject when disposing.

This presented several problems:

• The wrapped object reference might be null; but wrapping a null reference means things like if (wrapper == null) wouldn't behave properly; hence, the wrappers should override == and != operators to make null-checks work more intuitively.
• The COM threading model (STA) isn't completely compatible with .NET's (MTA); although in theory all managed calls get marshaled into a STA call, in practice the managed RCW could be "disconnected" from the underlying COM object, which basically means literally anything can throw a COMException - including a simple getter read. Wrapping everything in try/catch blocks would make for excessively redundant code, hence a generic invocation mechanism was implemented, to invoke a COM object's member while catching a COMException.
• Because the managed code implements IDisposable, we'll want an ObjectDisposedException to be thrown whenever a member is accessed on a disposed object. Hence, a general-purpose "throw if disposed" mechanism was implemented, to simplify the implementations.

Here is the base class from which all wrappers will be derived:

using System;
using System.Runtime.InteropServices;

namespace Rubberduck.VBEditor.DisposableWrappers
{
    public abstract class WrapperBase<T> : IDisposable
        where T : class 
    {
        private readonly T _item;
        private bool _isDisposed;

        protected WrapperBase(T item)
        {
            _item = item;
        }

        protected internal T Item
        {
            get
            {
                ThrowIfDisposed(_isDisposed);
                return _item;
            }
        }

        public bool IsNull
        {
            get
            {
                ThrowIfDisposed(_isDisposed);
                return _item == null;
            }
        }

        public static bool operator ==(WrapperBase<T> object1, WrapperBase<T> object2)
        {
            if (object1 != null && object1.IsNull)
            {
                return (object)object2 == null;
            }
            if (object2 != null && object2.IsNull)
            {
                return (object)object1 == null;
            }

            return ReferenceEquals(object1, object2);
        }

        public static bool operator !=(WrapperBase<T> object1, WrapperBase<T> object2)
        {
            return !(object1 == object2);
        }

        protected static TResult InvokeMemberValue<TResult>(Func<TResult> member)
        {
            try
            {
                return member.Invoke();
            }
            catch (COMException exception)
            {
                throw new WrapperMethodException(exception);
            }
        }

        protected static TResult InvokeMemberValue<T, TResult>(Func<T, TResult> member, T param)
        {
            try
            {
                return member.Invoke(param);
            }
            catch (COMException exception)
            {
                throw new WrapperMethodException(exception);
            }
        }

        protected static TResult InvokeMemberValue<T1, T2, TResult>(Func<T1, T2, TResult> member, T1 param1, T2 param2)
        {
            try
            {
                return member.Invoke(param1, param2);
            }
            catch (COMException exception)
            {
                throw new WrapperMethodException(exception);
            }
        }

        protected static void InvokeMember(Action member)
        {
            try
            {
                member.Invoke();
            }
            catch (COMException exception)
            {
                throw new WrapperMethodException(exception);
            }
        }

        protected static void InvokeMember<T>(Action<T> member, T param)
        {
            try
            {
                member.Invoke(param);
            }
            catch (COMException exception)
            {
                throw new WrapperMethodException(exception);
            }
        }

        protected static void InvokeMember<T1, T2>(Action<T1, T2> member, T1 param1, T2 param2)
        {
            try
            {
                member.Invoke(param1, param2);
            }
            catch (COMException exception)
            {
                throw new WrapperMethodException(exception);
            }
        }

        protected static void InvokeMember<T1, T2, T3>(Action<T1, T2, T3> member, T1 param1, T2 param2, T3 param3)
        {
            try
            {
                member.Invoke(param1, param2, param3);
            }
            catch (COMException exception)
            {
                throw new WrapperMethodException(exception);
            }
        }

        protected static void InvokeMember<T1, T2, T3, T4>(Action<T1, T2, T3, T4> member, T1 param1, T2 param2, T3 param3, T4 param4)
        {
            try
            {
                member.Invoke(param1, param2, param3, param4);
            }
            catch (COMException exception)
            {
                throw new WrapperMethodException(exception);
            }
        }

        protected static void ThrowIfDisposed(bool isDisposed)
        {
            if (isDisposed) { throw new ObjectDisposedException("Object has been disposed."); }
        }

        protected void ThrowIfDisposed()
        {
            ThrowIfDisposed(_isDisposed);
        }

        public void Dispose()
        {
            if (_isDisposed)
            {
                return;
            }

            Marshal.ReleaseComObject(_item);
            _isDisposed = true;
        }
    }
}

This base class allows me to wrap the VBIDE API types; here's the wrapper implementation for the Microsoft.Vbe.Interop.Window interface:

using System;
using Microsoft.Vbe.Interop;

namespace Rubberduck.VBEditor.DisposableWrappers
{
    public class Window : WrapperBase<Microsoft.Vbe.Interop.Window>, IDisposable
    {
        public Window(Microsoft.Vbe.Interop.Window window)
            : base(window)
        {
        }

        public void Close()
        {
            ThrowIfDisposed();
            InvokeMember(() => Item.Close());
        }

        public void SetFocus()
        {
            ThrowIfDisposed();
            InvokeMember(() => Item.SetFocus());
        }

        public void SetKind(WindowKind eKind)
        {
            ThrowIfDisposed();
            InvokeMember(kind => Item.SetKind((vbext_WindowType)kind), eKind);
        }

        public void Detach()
        {
            ThrowIfDisposed();
            InvokeMember(() => Item.Detach());
        }

        public void Attach(int lWindowHandle)
        {
            ThrowIfDisposed();
            InvokeMember(handle => Item.Attach(handle), lWindowHandle);
        }

        public VBE VBE
        {
            get
            {
                ThrowIfDisposed(); 
                return new VBE(InvokeMemberValue(() => Item.VBE));
            }
        }

        public Windows Collection
        {
            get
            {
                ThrowIfDisposed();
                return new Windows(InvokeMemberValue(() => Item.Collection));
            }
        }

        public string Caption
        {
            get
            {
                ThrowIfDisposed(); 
                return InvokeMemberValue(() => Item.Caption);
            }
        }

        public bool Visible
        {
            get
            {
                ThrowIfDisposed(); 
                return InvokeMemberValue(() => Item.Visible);
            }
        }

        public int Left
        {
            get
            {
                ThrowIfDisposed(); 
                return InvokeMemberValue(() => Item.Left);
            }
        }

        public int Top
        {
            get
            {
                ThrowIfDisposed(); 
                return InvokeMemberValue(() => Item.Top);
            }
        }

        public int Width
        {
            get
            {
                ThrowIfDisposed(); 
                return InvokeMemberValue(() => Item.Width);
            }
        }

        public int Height
        {
            get
            {
                ThrowIfDisposed(); 
                return InvokeMemberValue(() => Item.Height);
            }
        }

        public WindowState WindowState
        {
            get
            {
                ThrowIfDisposed();
                return InvokeMemberValue(() => (WindowState)Item.WindowState);
            }
        }

        public WindowKind Type
        {
            get
            {
                ThrowIfDisposed(); 
                return (WindowKind)InvokeMemberValue(() => Item.Type);
            }
        }

        public LinkedWindows LinkedWindows
        {
            get
            {
                ThrowIfDisposed();
                return new LinkedWindows(InvokeMemberValue(() => Item.LinkedWindows));
            }
        }

        public Window LinkedWindowFrame
        {
            get
            {
                ThrowIfDisposed(); 
                return new Window(InvokeMemberValue(() => Item.LinkedWindowFrame));
            }
        }

        public int HWnd
        {
            get
            {
                ThrowIfDisposed(); 
                return InvokeMemberValue(() => Item.HWnd);
            }
        }
    }
}

Notice I'm returning new wrapper instances whenever a member needs to return an object - that way I'm avoiding the incestuous object references of the wrapped object tree (pretty much everything in the VBIDE API has a VBE property that points to the parent/top-most object); the idea being that calling this member involves a new RCW and a reference counter increment, and therefore needs to be disposed/released as well:

    public VBE VBE
    {
        get
        {
            ThrowIfDisposed(); 
            return new VBE(InvokeMemberValue(() => Item.VBE));
        }
    }

So I'll be making it the calling code's responsibility to ensure Dispose gets called for every IDisposable wrapper involved - and contrary to Marshal.ReleaseComObject, I'll be getting compiler warnings for not calling IDisposable.Dispose when that call doesn't happen.

I'm also wrapping the enum types:

/// <summary>
/// Values compatible with <see cref="Microsoft.Vbe.Interop.vbext_WindowType"/> enum values.
/// </summary>
public enum WindowKind
{
    CodeWindow = 0,
    Designer = 1,
    Browser = 2,
    Watch = 3,
    Locals = 4,
    Immediate = 5,
    ProjectWindow = 6,
    PropertyWindow = 7,
    Find = 8,
    FindReplace = 9,
    Toolbox = 10,
    LinkedWindowFrame = 11,
    MainWindow = 12,
    ToolWindow = 15,
}

That way I can reuse everything when I need to wrap the VBIDE library for the VB6 IDE - I guess I'm going to need to copy all wrappers, and simply change the namespace in the generic type specification; I'll probably end up with Rubberduck.VBEditor.DisposableWrappers.VBA and Rubberduck.VBEditor.DisposableWrappers.VB6 namespaces, both with very similar types... that don't implement a common interface for their members... I can smell the nightmare coming.

The other implemented wrappers (that's all still WIP) can be found in my GitHub repository.

I'd like to receive constructive feedback on this approach before I get too far down the rabbit hole - is this a viable way of wrapping a COM interop library? Does anything look off? Am I falling into some "easy" trap with this implementation?

I specifically avoided implementing the wrapped interfaces, so as to avoid "leaking" any COM objects into my code base (e.g. there'd be a VBE property returning a Microsoft.Vbe.Interop.VBE COM object, vs here a VBE property returning a Rubberduck.VBEditor.DisposableWrappers.VBE). That's a good call, right?

Answer 1

There is two things that caught my attention.

Every public method / property of all implementing classes

needs to call ThrowIfDisposed(). Always. That's the requirements and what you started here is "manually" invoking that each and every time...

This seems to be quite a lot of work for something that could be accomplished significantly faster with Aspect-Oriented-Programming (an Interceptor or Decorator). I know Rubberduck is already using interceptors for logging ... I wonder whether it's possible to remove this tedious (and somewhat error prone) "manual" checking in favor of an "automated" check.

Needless overloads

You're passing an action and it's parameters to InvokeMember and InvokeMemberValue respectively. I don't understand why ... it should be easy to partially apply all invocations you want at the callsite of InvokeMember into an Action either way. Since you pass the parameters and an action and you have those available where you call InvokeMember why overload that.

Consider changing things like:

        InvokeMember(handle => Item.Attach(handle), lWindowHandle);

into a "cheaper" and partially applied action like this:

InvokeMember(() => Item.Attach(lWindowHandle));

It should not make a difference for the purpose of where the Exception is thrown and in gets rid of 6 methods in your WrapperBase that you won't need anymore... I think that's quite a benefit.

Answer 2

Just a flying review:

Have you tested this? The equality code isn't at all right. Firstly, never override == and != without overriding the object.Equals method. You'll end up with a check that will be different depending on which way you do the test - that's bad.

Secondly, your code can't handle this really easy test:

 var item1 = new WrapperBase<string>(null);
 Console.WriteLine(item1 == null);

That results in a StackOverflowException and this is the wrong site for that!

Your problem is object1 != null and object2 != null because you've defined != to just be !(==) you'll infinitely recurse. Use ReferenceEqual instead:

public static bool operator ==(WrapperBase<T> left, WrapperBase<T> right)
{
    if (!ReferenceEquals(left, null) && left.IsNull)
    {
        return ReferenceEquals(right, null);
    }
    if (!ReferenceEquals(right, null) && right.IsNull)
    {
        return ReferenceEquals(left, null);
    }
    return ReferenceEquals(left, right);
}

Answer 3

The wrapped object reference might be null

Then there's a bug right here:

public void Dispose()
{
    if (_isDisposed)
    {
        return;
    }

    Marshal.ReleaseComObject(_item);
    _isDisposed = true;
}

You need a null check against _item before you pass it to ReleaseComObject.

---

As mentioned in Vogel612's answer, the ThrowIfDisposed calls are easy to forget (especially in larger wrappers exposing lots of members).

The InvokeXxxx methods don't need to be static. By making them instance methods, you can move ThrowIfDisposed() calls there:

protected void Invoke(Action member)
{
    ThrowIfDisposed();
    try
    {
        member.Invoke();
    }
    catch (COMException exception)
    {
        throw new WrapperMethodException(exception);
    }
}

And now all the derived types need to remember is to Invoke everything.

---

The IDisposable implementation is a bit bare-bones: if a derived type needs to .Dispose its own resources, you're stuck. That's why the dispose pattern has this protected virtual void Dispose(bool disposing) member. The type also needs a finalizer as eurotrash mentioned, to ensure resources are disposed properly when the object gets finalized before Dispose was called.

The parameterized ThrowIfDisposed(bool) overload should be removed, too.