How to design an IDisposable that unconditionally needs to be disposed?

Question

Consider a class that implements IDisposable, and that has members in such a way that it will never become eligible for garbage collection when it is not disposed. And as it will not be garbage collected, it will not have the chance to use the destructor for cleaning up.

As a result, when it is not disposed (e.g. unreliable users, programming errors), resources will be leaked.

Is there a general approach how such a class can be designed to deal with such a situation or to avoid it?

Example:

using System;

class Program
{
    static void Main(string[] args)
    {
        new Cyclical();
        GC.Collect();
        GC.WaitForPendingFinalizers();
        Console.ReadKey();
    }
}

class Cyclical
{
    public Cyclical()
    {
        timer = new System.Threading.Timer(Callback, null, 0, 1000);
    }

    System.Threading.Timer timer;

    void Callback(object state)
    {
        Console.Write('.');  // do something useful
    }

    ~Cyclical()
    {
        Console.WriteLine("destructor");
    }
}

(Omitted IDisposable to keep example short.) This class uses a Timer to do something useful at certain intervals. It needs a reference to the Timer to avoid it is garbage collected.

Let’s assume the user of that class will not dispose it. As a result of the timer, somewhere some worker thread has a reference to the Cyclical instance via the callback, and as a result, the Cyclical instance will never become eligible for garbage collection, and its destructor will never run, and resources will leak.

In this example, a possible fix (or workaround) could be to use a helper class that receives callbacks from the Timer, and that does not have a reference, but only a WeakReference to the Cyclical instance, which it calls using that WeakReference.

However, in general, is there a design rule for classes like this that need to be disposed to avoid leaking resources?

---

For the sake of completeness, here the example including IDispose and including a workaround/solution (and with a hopefully less distracting name):

class SomethingWithTimer : IDisposable
{
   public SomethingWithTimer()
   {
      timer = new System.Threading.Timer(StaticCallback,
         new WeakReference<SomethingWithTimer>(this), 0, 1000);
   }

   System.Threading.Timer timer;

   static void StaticCallback(object state)
   {
      WeakReference<SomethingWithTimer> instanceRef
         = (WeakReference<SomethingWithTimer>) state;
      SomethingWithTimer instance;
      if (instanceRef.TryGetTarget(out instance))
         instance.Callback(null);
   }

   void Callback(object state)
   {
      Console.Write('.');  // do something useful
   }

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

   protected virtual void Dispose(bool disposing)
   {
      if (disposing)
      {
         Console.WriteLine("dispose");
         timer.Dispose();
      }
   }

   ~SomethingWithTimer()
   {
      Console.WriteLine("destructor");
      Dispose(false);
   }
}

• If disposed, the timer will be disposed.
• If not disposed, the object will become eligible for garbage collection.

Answer 1

One approach which can be helpful is to expose wrapper objects to the outside world, but don't hold any strong references to them internally. Either have the internal objects keep weak references to the wrappers, or else have each wrapper hold the only reference anywhere in the world to a finalizable object which in turn holds a reference to the "real" object [I don't like having any outside-world-facing objects implement Finalize, since objects have no control over who can call things like GC.SuppressFinalize() on them]. When all outside-world references to the wrapper object have disappeared, any weak references to the wrapper object will be invalidated [and can be recognized as such], and any finalizable object to which the wrapper held the only reference will run its Finalize method.

For event handlers which are guaranteed to be triggered on some period basis (e.g. timer ticks) I like the WeakReference approach. There's no need to use a finalizer to ensure the timer gets cleaned up; instead, the timer-tick event can notice that the outside-world link has been abandoned and clean itself up. Such an approach may also be workable for things whose only resources are subscriptions to rarely-fired events from long-lived (or static) objects, if those objects have a CheckSubscription event which fires periodically when a subscriber is added (the rate at which those events fire should depend upon the number of subscriptions). Event subscriptions pose a real problem is when an unbounded number of objects may subscribe to an event from a single instance of a long-lived object and be abandoned without unsubscribing; in that case, the number of abandoned-but-uncollectable subscriptions may grow without bound. If 100 instances of a class subscribe to an event from a long-lived object and are subsequently abandoned, and nothing else ever subscribes to that event, memory used by those subscriptions may never get cleaned up, but the quantity of wasted memory would be limited to those 100 subscriptions. Adding more subscriptions would at some point cause the CheckSubscription event to fire, which would in turn cause all the abandoned objects' subscriptions to get canceled.

PS--Another advantage of using weak references is that one can request that a WeakReference remain valid until the object to which it refers is 100% absolutely and irretrievably gone. It's possible for an object to appear abandoned and have its Finalize method scheduled, but then get resurrected and returned to active use even before its Finalize method actually executes; this can happen completely outside the object's control. If code holds a long weak reference to the public wrapper object, and if it makes sure the reference itself remains rooted, it can hold off on performing any cleanup until resurrection becomes impossible.

Answer 2

Under normal circumstances, cyclical references are not a problem for the GC, because it can detect them. But here, the whole object graph looks like this (based on looking at the reference source):

So, the problem isn't just the cycle, it's that you have a static field that references the cycle.

What you could do to fix this is to break the cycle between TimerCallback and Cyclical. In your sample code, that's simple: just changeCallback into a static method. In your real code, breaking the cycle might require more work, like creating a separate class for Callback, or maybe even using WeakReference.

What happens when you do this is that when Cyclical gets GCed, the finalizer of TimerHolder will run, which will stop the timer.

But you should still be careful with this, because the usual caveats for finalizers apply: you never know when will a finalizer run. Especially when you're not allocating much memory, it may take a very long time before it's run.

Answer 3

Consider a class that implements IDisposable, but when it is not disposed by its user, it will not become eligible for garbage collection, thus its destructor won’t run, and resources will be leaked.

Your presumption is incorrect. All classes are disposed and a properly written implementation of IDisposable will not leak resources.

To be absolutely clear,

• The garbage collector eventually collects all GC memory and does not free non-GC memory. Cyclical references are still collected, when available.
• IDisposable exists in order to free non-GC memory (and other resources) and has no role in freeing GC memory.

If Dispose is never called then all the GC memory will still be freed eventually. A properly written implementation of IDisposable will ensure that unmanaged resources will be freed too, even if Dispose is not called.

See http://msdn.microsoft.com/en-us/library/system.idisposable.aspx for details. The relevant quote is:

Because the IDisposable.Dispose implementation is called by the consumer of a type when the resources owned by an instance are no longer needed, you should either wrap the managed object in a SafeHandle (the recommended alternative), or you should override Object.Finalize to free unmanaged resources in the event that the consumer forgets to call Dispose.

If you have a reason to believe this is not so, please provide a reference and explanation why.

---

Two further points. There is no particular reason to expect that cyclical references cannot be garbage collected. Cycles cause a lot of problems for reference counting, but the GC has no such problems. The algorithm has 3 phases (mark, relocate, compact) and it finds live objects by tracing from 3 places (stack roots, GC handles and static data). Cycles that have no root are simply disposed.

There is a problem lurking here that wasn't mentioned. Although the GC guarantees the eventual finalisation of all objects, during which non-GC resources will be released, it does not guarantee the order in which this will happen. If there are order dependencies among the non-GC resources then relying on finalisation may result in leaks. The solution to that involves the proper allocation and management of non-GC resources, which is outside the scope of this question.

Answer 4

If your class contains a IDisposable field you'll need to make that also IDisposable so it can be explicitly Dispose its own fields. Or otherwise guarantee the Dispose happens eventually by adding a Finalize method.

Note that using the destructor for cleanup is not enough because they are not guaranteed to run (at least not in java).

class Cyclical: IDisposable
{
    public Cyclical()
    {
        timer = new System.Threading.Timer(Callback, null, 0, 1000);
    }

    System.Threading.Timer timer;

    public void Dispose()
    {
        timer.Dispose();
    }

    protected virtual void Finalize()
    {
        Dispose();
    }

    void Callback(object state)
    {
        Console.Write('.');  // do something useful
    }

    ~Cyclical()
    {
        Console.WriteLine("destructor");
        Dispose();
    }
}

Answer 5

To do this, first the fundamental rules of .Net garbage collection must be explained. An object is kept alive while any of the following is true:

• It is referenced in a static field
• It is referenced on the stack in any thread
• It is referenced by any object that is kept alive by any of these rules

So, if you have a static field in a class, any object placed in that field will be kept alive until that field is set to null. If you put a collection, array, ect, into that static field, any object in the collection or array will also be kept alive until it's removed from the collection, or the field is set to null.

To answer your question, though:

public class KeepAlive : IDisposable
{
   // Never garbage collected because its static
   private static HashSet<KeepAlive> doNotCollect = new HashSet<KeepAlive>();

   public KeepAlive()
   {
      // Right now this new object is kept alive because it's referenced on the
      // stack

      lock (doNotCollect)
         doNotCollect.Add(this);

      // Now this new object will never be collected until Dispose is called
      // because:
      // - doNotCollect isn't collected because its in a static field
      // - The new object is referenced by doNotCollect
   }

   public void Dispose()
   {
      lock (doNotCollect)
         doNotCollect.Remove(this);
   }
}

The above solution will work because it ensures that the new KeepAlive object is referenced in a static collection until Dispose() is called. One tiny caveat, though: Overriding Equals(), GetHashCode(), or the == operator, can cause problems. Thus, in this situation, you should not alter object equivalence behavior.

But, are you sure that's the behavior you want? It might be better to use the garbage collector to your advantage and warn about the resource leak. For example:

public class WarnWhenLeaked : IDisposable
{
    public void Dispose()
    {
       // Prevent the GC from calling ~WarnWhenLeaked()
       GC.SurpressFinalize(this);
    }

    ~WarnWhenLeaked()
    {
       Console.WriteLine("Leaked Resource!!!");
       this.Dispose();
    }
}

In the above class, when the object is Disposed, the call to the finalizer (~WarnWhenLeaked()) is disabled. Thus, if the object is garbage collected without calling Dispose(), it's able to log a warning, and release the resource.

The above situation might be better than permanently leaking the resource, because it could allow your program to recover from a bug.