Calling Dispose when setting IDisposable properties?

Question

I switched the FxCop rule today to point out any non-disposed IDisposables as errors, in a hope that it might help me track down some GDI leaks. Interestingly it pointed me to an instance that I'm not quite sure how to deal with:

public class CustomGoRectangle : GoRectangle
{
   public void DoStuff()
   {
      Pen p = new Pen(Color.Red, 4.0f);
      this.Pen = p;
   }

   public void DoOtherStuff()
   {
      Pen p = new Pen(Color.Blue, 4.0f);
      this.Pen = p;
   }

   public void Test()
   {
      this.Pen = Pens.Green;
      DoStuff();
      DoOtherStuff();
   }
}

Quick explanation. GoRectangle is within a 3rd party library and has a Pen property, it isn't IDisposable.

I set my pen in a number of places, like the contrived example above. The problem illustrated here is that calling DoStuff() I create a new pen, which is never disposed. Now I could call dispose on this.Pen if it's not null before assigning a new value, but then as Test() illustrates if a System Pen has been set this will just cause further problems. What's the actual pattern for dealing with situations like this?

Answer 1

You could manually call this.Pen.Dispose() before reassigning it.

Answer 2

No, you can't dispose all pens since system pens will throw ArgumentException if you try to dispose them (I just checked using reflector).

However, you can use reflection to get the value of the private field immutable in the Pen. If it's false you can safely dispose the pen. (I do not know if mono works in the same way)

Here is an extension method to help you:

public static void DisposeIfPossible(this Pen pen)
{
    var field = pen.GetType().GetField("immutable", BindingFlags.Instance|BindingFlags.NonPublic);
    if ((bool)field.GetValue(pen) == false)
        pen.Dispose();
}

Call it before assigning a new pen.

Answer 3

Here's the way I usually handle this sort of thing:

1) Complain to the library provider.

2) Create a disposition wrapper class that allows one to control whether its wrapped instance will actually be disposed when the wrapper is disposed. e.g. (ignoring Dispose(bool) noise):

public class DispositionWrapper<T> : IDisposable
    where T : IDisposable
{
    private readonly T _instance;
    private bool _allowDisposition;

    public DispositionWrapper(T instance, bool allowDisposition)
    {
        if (instance == null)
        {
            throw new ArgumentNullException("instance");
        }

        this._instance = instance;
        this._allowDisposition = allowDisposition;
    }

    public T Instance
    {
        get
        {
            return this._instance;
        }
    }

    public void Dispose()
    {
        if (this._allowDisposition)
        {
            this._instance.Dispose();
        }
    }
}

3) Use the disposition wrapper to allow early clean-up of instances for which disposition is known to be permissable. e.g.:

public class CustomGoRectangle : GoRectangle, IDisposable
{
    private DispositionWrapper<Pen> _ownedPen;

    public override Pen Pen
    {
        get
        {
            return this._ownedPen.Instance;
        }

        set
        {
            if (value == null)
            {
                this.OwnedPen = null;
            }
            else
            {
                this.OwnedPen = new DispositionWrapper<Pen>(value, false);
            }
        }
    }

    private DispositionWrapper<Pen> OwnedPen
    {
        get
        {
            return this._ownedPen;
        }

        set
        {
            if (this._ownedPen != null)
            {
                this._ownedPen.Dispose();
            }

            this._ownedPen = value;
        }
    }


    public void DoStuff()
    {
        this.OwnedPen = new DispositionWrapper<Pen>(new Pen(Color.Red, 4.0f), true);
    }

    public void DoOtherStuff()
    {
        this.OwnedPen = new DispositionWrapper<Pen>(new Pen(Color.Blue, 4.0f), true);
    }

    public void Test()
    {
        this.OwnedPen = new DispositionWrapper<Pen>(Pens.Green, false);
        this.DoStuff();
        this.DoOtherStuff();
    }

    public void Dispose()
    {
        if (this.OwnedPen != null)
        {
            this.OwnedPen.Dispose();
        }
    }
}

Unfortunately, this means that Pen instances assigned via the Pen property won't be cleaned up until they are finalized. If you are concerned about this, you might want to consider extending the disposition wrapper to detect whether the Pen can be cleaned up by reading the value of its immutable field via reflection, as mentioned jgauffin's answer.

Answer 4

The ideal approach for the situation where an IDisposable object is needed to use another object is to have a parameter which indicates whether an object which is given a reference to an IDisposable object should take ownership of it. An object which insists upon taking ownership of a passed-in IDisposable (e.g. StreamReader) can be as annoying as one which never accepts ownership. Given that GoRectangle does not accept ownership, whoever creates a pen for use with a GoRectangle will be responsible for disposing it. Approaches which assume any non-immutable pen should be disposed are dangerous, because there's no guarantee that any pen that's used by a GoRectangle that's being abandoned won't be shared by some other object that's still in scope.

Answer 5

Any Disposable object it should be disposed manually when no need it, and not relaying on GC.

In your case you can adopt 2 strategies:

1. create 2 pens outside the CustomGoRectangle (redPen and bluePen) or make them separate members and just assign it to the Pen member variable based on your flow.
2. create a pen when you need it do the work and dispose it than; and remove the Pen member variable.