views:

554

answers:

3

My class contains an object from Interop and calls a method on it which causes it to alloc stuff. It also exposes a method to free that stuff, so I expect I should call this in Dispose():

class MyClass : IDisposable
{
    private DllName.ComClassName comInstance;

    void SomeMethod()
    {
        comInstance = new DllName.ComClassName();
        comInstance.AllocStuff();
    }

    public void Dispose()
    {
        comInstance.FreeThatStuff();
    }
}

Now, I should expand all that to follow the Dispose pattern. I have no other disposable or unmanaged resources to release, so assuming comInstance is managed (isn't that what Interop does, wraps unmanaged into managed?), I think the pattern disolves to:

public void Dispose()
{
    if (comInstance != null)
    {
        comInstance.FreeStuff();
        comInstance = null;
    }
}

Which leaks unless I explicitly call Dispose() on instances of MyClass, which would make the Dispose pattern flawed? So does that mean comInstance must be unmanaged, and the pattern disolves to:

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

~MyClass()
{
    DisposeComInstance();
}

private void DisposeComInstance()
{
    if (comInstance != null)
    {
        comInstance.FreeStuff();
        comInstance = null;
    }
}

EDIT:

  1. To avoid cluttering my class with the full pattern, could I just seal my class?
  2. How do I know ComClassName (and in general any class) is unmanaged?
+2  A: 

It looks like you've almost got it nailed. I would fall back to the pattern where you have a protected virtual Disposing that takes a boolean parameter indicating if managed items should be disposed of. That way somebody coming behind you will continue to implement IDisposable properly.

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

~MyClass()
{
    this.Dispose(false);
}

protected virtual void Dispose(bool disposing)
{
    // if (disposing)
    // {
    //      // Managed
    // }

    if (comInstance != null)
    {
        comInstance.FreeStuff();
        comInstance = null;
    }

    // base.Dispose(disposing) if required
}
sixlettervariables
Meta-comment: be sure your Dispose(bool) can tolerate being called many times in a row. Also try to minimize the possibility of an exception being thrown (not always possible).
sixlettervariables
Meta-meta comment - as well as that, it's important that you never acquire locks or use locking during your unmanaged cleanup.
womp
+3  A: 

Ultimately you want this kind of pattern:

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

~MyClass()
{
    Dispose(false);
}

private void Dispose(bool disposing)
{
    if (disposing)
    {
        // Dispose of disposable objects here

    }

    // Other unmanaged cleanup here which will be called by the finalizer
    if (comInstance != null)
    {
         comInstance.FreeStuff();
         comInstance = null;
    }

    // Call base dispose if inheriting from IDisposable class.
    base.Dispose(true);
}

For a great article on why, check out Implementing IDisposable and the Dispose pattern properly.

womp
-1, You need to move the unmanaged objects outside of the if (disposing) block. Unmanaged should always be cleaned up.
sixlettervariables
Gah! You're right, I copied it from some of my code that was using components that had unmanaged code but implemented dispose, and edited in the OP's example... paste for the lose. Fixed.
womp
+1, correctly implemented ;-D
sixlettervariables
+2  A: 

Firstly, I agree with those that suggest having a finalizer as a backup, but try to avoid it ever being called by explicitly calling myClass.Dispose or through 'using'.

e.g.

var myClass = new MyClass()
try
{
   //do stuff
}
finally
{
   myClass.Dispose();
}

or

using (var myClass = new MyClass())
{
   //do stuff
}

Marshall.ReleaseComObject

If you are using lots of COM objects, I also suggest you use Mashall.ReleaseComObject(comObj) to explicitly clean up references to the RCW.

So, the code like this suggested elsewhere:

if (comInstance != null)
{
    comInstance.FreeStuff();
    comInstance = null;
}

would become:

if (comInstance != null)
{
    comInstance.FreeStuff();

    int count = Marshall.ReleaseComObject(comInstance);
    if (count != 0)
    {
            Debug.Assert(false, "comInstance count = " + count);
            Marshal.FinalReleaseComObject(comInstance);
    }

    comInstance = null;
}

While checking the return value of ReleaseComObject() is not strictly necessary, I like to check it to make sure things are being incremented/decremented as expected.

The 2-dot rule

If you decide to use this, something to be aware of is that some code may need to be refactored to properly release your COM objects. One in particular is what I call the 2-dot rule. Any line using COM objects that contain 2 dots needs close attention. For example,

var name = myComObject.Address.Name;

In this statement we get a reference to the Address COM object incrementing its RCW reference count, but we don't have an opportunity to call ReleaseComObject. A better way to do it would be to break it down (try..finally omitted for clarity):

var address = myComObject.Address;
var name = address.Name;
MyReleaseComObject(address);

where MyReleaseComObject is a utility method wrapping my count check and FinalReleaseComObject() from above.

Phil Haselden