Automated test for memory leaks? (Part 2)

In my earlier post I discussed a not-so-optimal algorithm for identifying memory leaks within an application.  Notwithstanding the issues presented in that post, the test was largely reactive in nature.  So how do we make a proactive test, namely, how do we stop leaks from ever occurring?  Attack the root!

Ok, that might be a little overzealous as there are a variety of ways to leak memory.  However, in my experience (and the experience of several others in the blogosphere) improperly used event handlers are the most frequent cause of leaks.  Fortunately, they’re also some of the easiest to fix as long as you know where the problems are.

Using Mono, I wrote a test that will scan all of our dlls and keeps a tally of how many times an event handler has been subscribed to and how many times it has been unsubscribed from.  Ideally, the overall tally would be zero.

Pseudocode for the algorithm:

foreach assembly

foreach type (class) in assembly

foreach method in type

foreach instruction in method

if instruction is event subscription/unsubscription

find event owner

increment/decrement tally for event

analyze tallies and find positive count events

store error message for type

Assert that error message is empty

As simple as the pseudocode makes it seem, it’s not a trivial test by any means.  (Also, even though there is a quadruple nested foreach, it’s still linear run time since we never analyze a method more than once).

Consider the following lines of event-related instruction:

Unloaded += OnUnload;

contextMenu.Showing += ContextMenuShowing;

When parsing the instructions with mono, you see the instructions the way the compiler does.   I don’t know about you, but notwithstanding my rudimentary courses in compilers I don’t have OpCodes memorized.  And unfortunately I haven’t found anything that explicitly identifies an instruction as being an event subscription.  When analyzing the first event subscription(Unloaded), what you see is the following:

Pretty obscure, but what we have is “System.Void System.Windows.FrameworkElement::add_Unloaded(System.Windows.RoutedEventHandler)” (Likewise when you do subtraction it will contain “::remove_”).  We can use this to extract our event name as well as whether it’s being subscribed to or removed from.

Now we have to find out if the event belongs to a variable or not (I’ll go into why this is important later.)  We can traverse the instructions via the “Previous” member.  By analyzing the opCodes and operands we can determine whether a variable is being loaded and extract its name.  This can be tricky because local variable, member variables and method parameters all have different OpCodes.  Also, consider static variables and owners that are being cast to another type.  All of these conditions require some tricky logic (and I’m still finding various conditions which need consideration).

So why do we need to find the owner?  Remember that we are finding the delta between adding and removing handlers for events and if there is a positive net delta then we have an “error”.  With that in mind consider the following example method:

public void Initialize()
{
      	textbox1.TextChanged += OnTextChanged;
	textbox2.TextChanged += OnTextChanged;
	textbox3.TextChanged += OnTextChanged;
	textbox4.TextChanged += OnTextChanged;
	textbox5.TextChanged += OnTextChanged;

	//do stuff

	textbox1.TextChanged -= OnTextChanged;
	textbox1.TextChanged -= OnTextChanged;
	textbox1.TextChanged -= OnTextChanged;
	textbox1.TextChanged -= OnTextChanged;
	textbox1.TextChanged -= OnTextChanged;
}

If we just counted the number of times “TextChanged” was subscribed to/unsubscribed from, the previous block of code would pass even though there are 4 handlers (textbox2-textbox5) still listening.  So to alleviate this issue, I find the owner (when possible) and create a key based on the owner and the event.

Now, there are some imperfections with this test.

First, I only analyze one class at a time.  So if an event is subscribed to in one class and detached from in another, the test will fail.  I did this intentionally because chances are either the event owner or the handler was local to the scope of the class, so chances of having all necessary handles in order to correctly unsubscribe are slim.

Second, (and this wouldn’t be difficult) an improvement would be to consider the delegate name as well in case multiple delegates were attached to a single event.

Third, because we assume an instruction is an event if the operand contains “::add_” or “::remove_” there could be some unexpected results.  I haven’t seen any yet, but just something to consider.

Fourth, there are some events that won’t cause leaks, e.g. a local delegate listening to a local event.  However, it is always good practice to remove any handler that is explicitly attached (vs. handlers that are attached in InitializeComponent(), which will automatically detached when .Dispose() is invoked.)

Also, unless you’re doing perfect TDD, you’ll be adding this to an existing codebase and will probably find several hundred (if not thousand) errors. What I’ve done with our code is created a baseline and then any new errors that are checked into the repository will break our automated build.  As I process the “errors” for each class, I check if they are “existing”.  If they are and their count hasn’t increased, I’ll write them out to file again.  If they aren’t, or if the count increases the test fails.  Doing this ensures that we can only improve as existing “errors” will be removed from the files as they are fixed and new errors won’t be added to the baseline.

Below is my code for parsing this instructions:

if (typeDef.HasCustomAttributes && typeDef.CustomAttributes.Select(att => att.AttributeType.FullName).Any(name => name.Contains("NUnit.Framework.TestFixtureAttribute")))
{
	return;
}
ReadExistingErrors(typeDef);
foreach (MethodDefinition methodDef in typeDef.Methods)
	{
		if (methodDef.HasBody && !methodDef.Name.Contains("InitializeComponent"))
		{
			methodDef.Body.SimplifyMacros();
			foreach (Instruction instruction in methodDef.Body.Instructions)
			{
				if (instruction.OpCode.Code != Code.Callvirt && instruction.OpCode.Code != Code.Call)
				{
					continue;
				}
				string operand = instruction.Operand.ToString();
				if (operand.Contains("::add_"))
				{
					AddEventToList(operand, GetEventOwner(instruction), true);
				}
				else if (operand.Contains("::remove_"))
				{
					AddEventToList(operand, GetEventOwner(instruction), false);
				}
			}
			methodDef.Body.OptimizeMacros();
		}
	}
As always, comments welcome.

 

Automated test for memory leaks? (Part 1)

If you’ve been there you’ll know what I’m talking about.  You’ve spent sickening amounts of time identifying the cause of a defect and just as much time fixing it.  You’re feeling pretty good about yourself and enjoy the respite of a calm mind knowing that the beast is slayed never to haunt your nightmares again.  A few days later, however, you’re fortunate enough to discover another developer has made changes that reintroduced the defect.  Anyone with a shred of testing experience would say, “you should’ve written a regression test!”.

I completely agree.  But what about when you can’t?

Heresy?  Maybe, but then again I’m not James Gosling or Bill Gates so magical life altering algorithms don’t spew forth from my orifices.  I’m just a good looking 20-something year old who happens to write code.  The defects I’m talking about specifically are memory leaks.

I’ve read through several dozens of peoples’ attempts at creating an automated test for memory leaks.  They all follow the same general format:

1. Start your app and reach some point where all of the necessary items are cached
2. Take a snapshot of the memory being used (some might even invoke GC.Collect here)
3. Execute the code in question and return to the baseline
4. Invoke whatever cocktail of GC.Collect call you feel are necessary
5. Take a snapshot again
6. Compare the results and verify memory hasn’t increased beyond an acceptable amount.

There are two pretty big issues:  First, automating step one could be a feat in and of itself and second, we’d have to repeat step three a number of times to ensure the bloat in memory (if any) is indeed caused by the accused method and not by some delayed execution of your app that the OS queued for performance reason, which only exacerbates the issue of uncontrolled delayed execution.  Ignoring those tidbits, there are some pretty clever algorithms that people have derived that make this work for specific applications and scenarios.  However, ultimately we need to remember a key principle about the .Net garbage collector:

If an object is marked for Garbage Collection, .Net only guarantees that it will be collected, is doesn’t guarantee when it will be collected.

People often think that calling

GC.Collect(GC.MaxGeneration, GCCollectionMode.Forced);
GC.WaitForPendingFinalizers();

will give them a good benchmark for their memory.  In truth, this may work some of the time, but before you go out and try it I would suggest reading this post about how the Garbage Collector works.  For all intents and purposes, the OS is in control of performance and not us.  As much as we’d like to force the GC to do its job, the only thing we can do it tell it to do its job and hope it does it soon.

<Devin’s assumption>

The reason programs like .Net Memory Profiler can invoke the GC without problems is because they have hooks in every instruction of the program (which is why the memory bloats so badly when you profile…look at the private bytes of your app in task manager next time you profile it).  It can halt execution of any instruction thereby giving the GC an unobstructed path towards memory reclamation heaven.

</Devin’s assumption>

In a nutshell, any memory snapshots we take before or after calls to GC.Collect will rarely be completely accurate.  BUT, if you do choose to put in the work to create this type of test, the biggest issue would be with false failures.  If you take a snapshot after you’ve executed your offending code and it doesn’t seem to have spiked, you can be assured the GC collected (or that you didn’t write your test correctly 🙂 )

In my next post I’ll talk about an alternative that we can do to give us some peace of mind.