Monday, March 19, 2007

Delegates rock. But there's a dark side to them that can cause non-obvious bugs. Read on if you dare!

The Problem

One of the hazards of using delegates is the potential memory leak that can occur if an instance method is referenced by a delegate that is rooted. This most often occurs with static events but can easily happen in other scenarios. The problem is that a delegate which references an instance method has to maintain a strong reference to an object instance that the method can be called on. If it doesn't maintain a strong reference, the instance might be garbage collected and the delegate won't be able to call the instance method (since there's no longer an instance to call it on). Let's look at a simple example.

private delegate void MessageWriter(string text);

public class ConsoleWriter
{
  public void WriteToConsole(string text)
  {
    Console.Write(text);
  }
}

// Client code...
ConsoleWriter writerObj = new ConsoleWriter();
MessageWriter writerDel = new MessageWriter(writerObj.WriteToConsole);

writerDel("Hello, World!");

Every delegate has two properties: Target and Method. If the delegate references an instance method, Target refers to the instance upon which the referenced method will be called. In other words, Target becomes the "this" pointer. Method points to a System.Reflection.MethodInfo object that represents the method that will be called when the delegate is invoked.

(NOTE: There is a bit of overhead the first time that the Method property is accessed because its value is lazily evaluated. It isn't actually used during normal delegate invocation — internal structures are used instead.)

Here is what the code above looks like at runtime:

diagram

Because Target maintains a strong reference to the "writerObj" instance, it is possible to rewrite the client code like this:

// Client code...
ConsoleWriter writerObj = new ConsoleWriter();
MessageWriter writerDel = new MessageWriter(writerObj.WriteToConsole);

writerObj = null;
GC.Collect();

writerDel("Hello, World!");

The message is still printed to the console because, thanks to the strong reference, "writerObj" is never actually garbage collected — even though the code explicitly forces a garbage collection. At first glance, this looks like a great feature. In fact, it is this strong reference that makes all of the closure stuff I've been writing about possible. But it has a dark side. Take a look at this slightly more complicated example:

using System;
using Microsoft.Win32;

class DisplaySettingsListener
{
  byte[] m_ExtraMemory = new byte[1000000];

  public DisplaySettingsListener()
  {
    SystemEvents.DisplaySettingsChanged += new EventHandler(ehDisplaySettingsChanged);
  }

  void ehDisplaySettingsChanged(object sender, EventArgs e)
  {
  }
}

class Program
{
  static void DisplayMemory()
  {
    Console.WriteLine("Total memory: {0:###,###,###,##0} bytes", GC.GetTotalMemory(true));
    Console.WriteLine();
  }

  static void Main()
  {
    DisplayMemory();

    for (int i = 0; i < 5; i++)
    {
      Console.WriteLine("--- New Listener #{0} ---", i + 1);

      DisplaySettingsListener listener = new DisplaySettingsListener();

      listener = null;
      GC.Collect();

      DisplayMemory();
    }

    Console.WriteLine("--- press any key to quit ---");
    Console.WriteLine();
  }
}

When I run this console application on my machine, I get the following output:

Total memory: 427,774 bytes

--- New Listener #1 ---
Total memory: 1,436,608 bytes

--- New Listener #2 ---
Total memory: 2,436,684 bytes

--- New Listener #3 ---
Total memory: 3,436,760 bytes

--- New Listener #4 ---
Total memory: 4,436,836 bytes

--- New Listener #5 ---
Total memory: 5,436,928 bytes

--- press any key to quit ---

Obviously, this application results in a nasty memory leak. The leak happens because the DisplaySettingsListener class adds a handler for the SystemEvents.DisplaySettingsChanged event. Under the hood, an event is simply property-like syntactic sugar for a delegate so, like any delegate referencing an instance method, it maintains a strong reference to the DisplaySettingsListener instance. To complicate matters, the SystemEvents.DisplaySettingsChanged event is static which means that its delegate will exist until the current AppDomain is unloaded. Since this is the main AppDomain, that means the DisplaySettingsListener instances will exist until the application is closed. Granted, it's a contrived example but imagine that DisplaySettingsListener is actually a dialog in a real application that listens to SystemEvents.DisplaySettingsChanged to ensure that it displays some data properly even if the settings are changed. This would be a pretty serious bug.

The Solution

The solution is to ensure that any delegates which might result in such a leak are unhooked before we release all references to an instance. With this in mind, we can correct the last example by adding a new method to DisplaySettingsListener like this:

public void CleanUp()
{
  SystemEvents.DisplaySettingsChanged -= new EventHandler(ehDisplaySettingsChanged);
}

Then, we can call CleanUp immediately before setting the "listener" instance to null.

DisplaySettingsListener listener = new DisplaySettingsListener();

listener.CleanUp();
listener = null;
GC.Colelct();

With those changes in place, I get the following output when running the application:

Total memory: 427,774 bytes

--- New Listener #1 ---
Total memory: 436,584 bytes

--- New Listener #2 ---
Total memory: 436,584 bytes

--- New Listener #3 ---
Total memory: 436,584 bytes

--- New Listener #4 ---
Total memory: 436,584 bytes

--- New Listener #5 ---
Total memory: 436,584 bytes

--- press any key to quit ---

So, the memory leak is fixed. To make this more fluid, we could use the .NET dispose pattern, implement IDisposable on DisplaySettingsListener and remove the event handler inside of the Dispose() method but this works OK for now. So, all is happy and merry except for one minor detail: this solution sucks.

<rant>
The problem that I have with this solution is that it forces developers to babysit the garbage collector. We've been told that the garbage collector is supposed to be our friend. In fact, when being introduced to the .NET framework, we are told to release an object and forget about it. "Let the garbage collector handle it" we're told. For many of us, this is a struggle against our natural instinct to clean up but, eventually, we overcome and learn to trust the garbage collector. And yet, here it is creating memory problems. Having to remember to unhook a handler from a delegate isn't really that different than having to remember to call free on some pointer allocated with malloc. I thought that the garbage collector was supposed to protect us from bugs, not introduce a whole new set of bugs! ARRGGHH!!!
</rant>

The Ideal Solution

The best solution to this problem is to introduce a notion of "weak delegates" into the CLR. A weak delegate would create a weak reference to its Target rather than a strong one. Then, when the Target is garbage collected, the runtime would remove the delegate from any delegate chains that it is currently a part of. And, if there are any direct references to the delegate, they would do nothing when invoked. I'm not sure what the C# syntax would look like for such an animal. Maybe something like this:

SystemEvents.DisplaySettingsChanged += new weak EventHandler(ehDisplaySettingsChanged);

I am not the first person to suggest something like this. In fact, there have been several. Unfortunately, because the .NET framework will be built upon the 2.0 version of the CLR for the foreseeable future, there probably won't be anything added for quite some time (if ever).

So, we developers are forced to roll our own solution to this problem. There have been several attempts made and not all of them have been successful. It turns out that getting the right balance of efficiency, memory use and elegance is pretty tricky. Next time, we'll take an in-depth look at one potential solution that I've found to be successful.

posted on Monday, March 19, 2007 12:00:13 PM (Pacific Standard Time, UTC-08:00)  #    Comments [1]

kick it on DotNetKicks.com