11
\$\begingroup\$

The disposable wrappers worked exactly as intended... and that turned out being a huge mistake:

Error: COM object that has been separated from its underlying RCW cannnot be used

That's because the .net runtime creates a Runtime Callable Wrapper per-type, not per-instance*; it took a number of refactorings and adjustments just to be able to get the project to build and run, and then over 900 tests were broken and my life was a nightmare.

Then I had an idea: since .net wouldn't let me destroy objects that were otherwise ready to be collected, what if I ditched IDisposable and moved on to a hierarchical teardown strategy?

Enter ISafeComWrapper:

namespace Rubberduck.VBEditor.SafeComWrappers
{
    public interface ISafeComWrapper
    {
        /// <summary>
        /// Releases all COM objects.
        /// </summary>
        void Release();
    }
}

In the Extension class (which you may recall seeing here), we Release the entire VBE object graph, like this:

private void ShutdownAddIn()
{
    if (_app != null)
    {
        _app.Shutdown();
        _app = null;
    }

    if (_kernel != null)
    {
        _kernel.Dispose();
        _kernel = null;
    }

    _ide.Release();
    _isInitialized = false;
}

So here's the SafeComWrapper<T> base class:

using System;
using System.Diagnostics.CodeAnalysis;
using System.Runtime.InteropServices;

namespace Rubberduck.VBEditor.SafeComWrappers
{
    public abstract class SafeComWrapper<T> : ISafeComWrapper, IEquatable<SafeComWrapper<T>> 
        where T : class 
    {
        protected SafeComWrapper(T comObject)
        {
            _comObject = comObject;
        }

        public abstract void Release();

        private readonly T _comObject;
        public T ComObject { get { return _comObject; } }
        public bool IsWrappingNullReference { get { return _comObject == null; } }

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

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

        /// <summary>
        /// <c>true</c> when wrapping a <c>null</c> reference and <see cref="other"/> is either <c>null</c> or wrapping a <c>null</c> reference.
        /// </summary>
        protected bool IsEqualIfNull(SafeComWrapper<T> other)
        {
            return (other == null || other.IsWrappingNullReference) && IsWrappingNullReference;
        }

        public override bool Equals(object obj)
        {
            return Equals(obj as SafeComWrapper<T>);
        }

        public abstract bool Equals(SafeComWrapper<T> other);
        public abstract override int GetHashCode();

        public static bool operator ==(SafeComWrapper<T> a, SafeComWrapper<T> b)
        {
            if (ReferenceEquals(a, null) && ReferenceEquals(b, null))
            {
                return true;
            }
            return !ReferenceEquals(a, null) && a.Equals(b);
        }

        public static bool operator !=(SafeComWrapper<T> a, SafeComWrapper<T> b)
        {
            return !(a == b);
        }

        [SuppressMessage("ReSharper", "RedundantCast")]
        [SuppressMessage("ReSharper", "ForCanBeConvertedToForeach")]
        [SuppressMessage("ReSharper", "LoopCanBeConvertedToQuery")]
        protected int ComputeHashCode(params object[] values) // incurs boxing penalty for value types
        {
            unchecked
            {
                const int initial = (int)2166136261;
                const int multiplier = (int)16777619;
                var hash = initial;
                for (var i = 0; i < values.Length; i++)
                {
                    hash = hash * multiplier + values[i].GetHashCode();
                }
                return hash;
            }
        }
   }
}

The derived types all implement IEquatable<WhateverTheTypeIs> on top of having to override GetHashCode, the base Equals<SafeComWrapper<T>> method and the Release method - here's the AddIn wrapper implementation:

using System;
using System.Runtime.InteropServices;

namespace Rubberduck.VBEditor.SafeComWrappers.VBA
{
    public class AddIn : SafeComWrapper<Microsoft.Vbe.Interop.AddIn>, IEquatable<AddIn>
    {
        public AddIn(Microsoft.Vbe.Interop.AddIn comObject) 
            : base(comObject)
        {
        }

        public string ProgId
        {
            get
            {
                return IsWrappingNullReference ? null : InvokeResult(() => ComObject.ProgId);
            }
        }

        public string Guid
        {
            get { return IsWrappingNullReference ? null : InvokeResult(() => ComObject.Guid); }
        }

        public VBE VBE
        {
            get { return new VBE(InvokeResult(() => IsWrappingNullReference ? null : ComObject.VBE)); }
        }

        public AddIns Collection
        {
            get { return new AddIns(InvokeResult(() => IsWrappingNullReference ? null : ComObject.Collection)); }
        }

        public string Description
        {
            get
            {
                return IsWrappingNullReference ? string.Empty : InvokeResult(() => ComObject.Description);
            }
            set
            {
                Invoke(() => ComObject.Description = value);
            }
        }

        public bool Connect
        {
            get
            {
                return !IsWrappingNullReference && InvokeResult(() => ComObject.Connect);
            }
            set
            {
                Invoke(() => ComObject.Connect = value);
            }
        }

        public object Object // definitely leaks a COM object
        {
            get
            {
                return IsWrappingNullReference ? null : InvokeResult(() => ComObject.Object);
            }
            set
            {
                Invoke(() => ComObject.Object = value);
            }
        }

        public override void Release()
        {
            if (!IsWrappingNullReference)
            {
                Marshal.ReleaseComObject(ComObject);
            }
        }

        public override bool Equals(SafeComWrapper<Microsoft.Vbe.Interop.AddIn> other)
        {
            return IsEqualIfNull(other) || (other != null && other.ComObject.ProgId == ProgId && other.ComObject.Guid == Guid);
        }

        public bool Equals(AddIn other)
        {
            return Equals(other as SafeComWrapper<Microsoft.Vbe.Interop.AddIn>);
        }

        public override int GetHashCode()
        {
            return IsWrappingNullReference ? 0 : ComputeHashCode(ProgId, Guid);
        }
    }
}

The collection types implement IEnumerable<WhateverTheWrapperTypeIs> on top of that, and are responsible for releasing their children - e.g. here's the AddIns class:

using System;
using System.Collections;
using System.Collections.Generic;
using System.Runtime.InteropServices;

namespace Rubberduck.VBEditor.SafeComWrappers.VBA
{
    public class AddIns : SafeComWrapper<Microsoft.Vbe.Interop.Addins>, IEnumerable<AddIn>, IEquatable<AddIns>
    {
        public AddIns(Microsoft.Vbe.Interop.Addins comObject) : 
            base(comObject)
        {
        }

        public int Count
        {
            get { return IsWrappingNullReference ? 0 : InvokeResult(() => ComObject.Count); }
        }

        public object Parent // todo: verify if this could be 'public Application Parent' instead
        {
            get { return IsWrappingNullReference ? null : InvokeResult(() => ComObject.Parent); }
        }

        public VBE VBE
        {
            get { return IsWrappingNullReference ? null : new VBE(InvokeResult(() => ComObject.VBE)); }
        }

        public AddIn Item(object index)
        {
            return new AddIn(InvokeResult(() => ComObject.Item(index)));
        }

        public void Update()
        {
            Invoke(() => ComObject.Update());
        }

        public override void Release()
        {
            if (!IsWrappingNullReference)
            {
                for (var i = 1; i <= Count; i++)
                {
                    Item(i).Release();
                }
                Marshal.ReleaseComObject(ComObject);
            }
        }

        public override bool Equals(SafeComWrapper<Microsoft.Vbe.Interop.Addins> other)
        {
            return IsEqualIfNull(other) || (other != null && ReferenceEquals(other.ComObject.Parent, Parent));
        }

        public bool Equals(AddIns other)
        {
            return Equals(other as SafeComWrapper<Microsoft.Vbe.Interop.Addins>);
        }

        public override int GetHashCode()
        {
            return IsWrappingNullReference ? 0 : ComputeHashCode(Parent);
        }

        IEnumerator IEnumerable.GetEnumerator()
        {
            return InvokeResult(() => ComObject.GetEnumerator());
        }

        IEnumerator<AddIn> IEnumerable<AddIn>.GetEnumerator()
        {
            return new ComWrapperEnumerator<AddIn>(ComObject);
        }
    }
}

This beauty seems to work perfectly.. but I don't like that I'm having to rely on ComObject in GetHashCode and Equals when I'm wrapping collection types and other types that don't really have any immutable data - because with COM interop, there's no guarantee that the returned hash code will be the same throughout the lifetime of the RCW... but I don't think I have any other options. Do I?

I don't like that I have an insignificant stack trace for whatever exception gets thrown in Invoke or InvokeResult, but I'm guessing that's the price to pay for lazy-accessing the underlying COM object.

Does anything else look wrong?


*By all means please correct me if I'm wrong here - this area of .net is shrouded in thick clouds, and the web keeps contradicting itself about it.

\$\endgroup\$
  • \$\begingroup\$ About the creation of rcw take a look at stackoverflow.com/a/5771301/2655508 \$\endgroup\$ – Heslacher Oct 6 '16 at 16:55
  • \$\begingroup\$ @Heslacher saw that, thanks; Reference count is incremented each time the object is requested from within COM object is actually what made me go and dispose/release everything (one COM object retrieved = one COM object released) - which led to the catastrophe depicted in the introduction of this post. An RCW per object makes sense.. but contradicts the examples in the post (accessing VBE.ActiveVBProject twice returns the same object twice, and therefore one RCW - releasing twice sends everything down in flames apparently. \$\endgroup\$ – Mathieu Guindon Oct 6 '16 at 18:13
  • \$\begingroup\$ That said, with this solution I now have a clean exit when the host document is closed before Rubberduck tears down, which is already a massive improvement. \$\endgroup\$ – Mathieu Guindon Oct 6 '16 at 18:15
  • \$\begingroup\$ Why do you catch different specific exceptions and then throw new WrapperMethodException for both? Does that exception have some knowledge of COMExceptions? Looks like really great code btw. \$\endgroup\$ – RobH Oct 9 '16 at 19:56
  • 1
    \$\begingroup\$ @RobH good question - I actually ended up outright removing that WrapperMethodException and the Invoke/InvokeResult methods too, since they really only existed so that I would be sure to ThrowIfDisposed()... but since I'm no longer implementing IDisposable it was all moot. Thanks! \$\endgroup\$ – Mathieu Guindon Oct 9 '16 at 19:59
2
\$\begingroup\$
public static bool operator ==(SafeComWrapper<T> a, SafeComWrapper<T> b)
{
    if (ReferenceEquals(a, null) && ReferenceEquals(b, null))
    {
        return true;
    }
    return !ReferenceEquals(a, null) && a.Equals(b);
}

I think that this should be done a little differently, this looks overly complicated and a bit confusing.

I was thinking that a ternary statement would make this much cleaner and less redundant with the ReferenceEquals(a, null)

it would look something like this.

public static bool operator ==(SafeComWrapper<T> a, SafeComWrapper<T> b)
{
    return ReferenceEquals(a, null) ? ReferenceEquals(b, null) : a.Equals(b);
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.