patterncsharpMinor
Wrapping COM objects with IDisposable
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
To fix this, I've undertaken the task of completely wrapping the VBIDE API with managed types that implement
This presented several problems:
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
{
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 anullreference means things likeif (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 intry/catchblocks would make for excessively redundant code, hence a generic invocation mechanism was implemented, to invoke a COM object's member while catching aCOMException.
- Because the managed code implements
IDisposable, we'll want anObjectDisposedExceptionto 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
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
Consider changing things like:
into a "cheaper" and partially applied action like this:
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.
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.