HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Wrapping COM objects with IDisposable

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
objectswithwrappingcomidisposable

Problem

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 : IDisposable
where T : class
{
private readonly T _item;
private bool _isDisposed;

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

protected internal T Item
{
get
{

Solution

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.

Code Snippets

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

Context

StackExchange Code Review Q#142916, answer score: 4

Revisions (0)

No revisions yet.