Ayende @ Rahien

Refunds available at head office

Some finalizer pitfalls

The use of finalizer to ensure eventual release of resources is recommended, but one thing that is not discussed often is the implications of adding a finalizer.

No, I am not talking about the extra wait time before the class memory is released, this is fairly well known. I am talking about two major additional issues that don't get enough attention, in my opinion.

Specifically, invalid instances and threading.

An object that has a finalizer will may be executed in more than one thread. Luckily, the CLR ensure that our finalizer will not be able to run if there is any code that is using the class, so it is not as bad as it seems at first.

However, something that make absolute sense was a bit of a surprise to me, when I figured it out, finalizers will run for invalid instances as well. What do I mean by that? Invalid instance is an object whose ctor threw an exception. It is in invalid state.

But, before it threw an exception, it might have acquired resources that needs to be released, so it goes to the GC finalizer as well. In fact, since you can't call Dispose on such an object, it must go to the finalizers for cleanup.

Remember the invalid state bit part? This is important!

Your object is in invalid state, and it is asked to cleanup itself. If it can't do so even in invalid state, an exception will be thrown. This exception on the finalizer thread will kill the application.

Here is the proof of concept code:

public class BadClass : IDisposable
{
	FileStream fs;

	public BadClass(int i)
	{
		if (i%2 == 0)
			throw new InvalidDataException("evens are bad for your health");
		fs = new FileStream(Path.GetTempFileName(), FileMode.Create);
	}

	~BadClass()
	{
		fs.Dispose();
	}

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

And the code to execute it:

for (int i = 0; i < 1000; i++)
{
	try
	{
		using (new BadClass(i))
		{

		}
	}
	catch (Exception)
	{
	}
}

Trying to figure out what is going on in this scenario is tough. Going back to the threaded aspect of it. What this means is that such an object is a time bomb, and it will blow up at some future date to kill your application, but you are not likely to know what exactly caused it to be this way.

In my situation, I figured it out by putting new StackTrace(true) in the constructor of the object, and breaking into the debugger on the finalizer.  But until it occurred to me to look for it there...

Comments

Haacked
07/28/2008 03:11 PM by
Haacked

Good tip! I generally avoid making any assumptions about my data in the finalizer. I'd probably change that code to something like this:

~BadClass()

{

if(fs != null) {

  fs.Dispose();

}

}

Or if there's any place where fs gets set to null, I'd do it this way to avoid potential threading issues:

~BadClass()

{

var thefs = fs;

if(thefs != null) {

  thefs.Dispose();

}

}

Ayende Rahien
07/28/2008 03:56 PM by
Ayende Rahien

Haacked,

When the finalizer run, no other code can touch the class

Haacked
07/28/2008 04:17 PM by
Haacked

True for now. But I wouldn't rely on that in the future and I tend to be paranoid and defensive when I code.

For example, the CLR team has mentioned they may add multiple finalizer threads in the future. See this comment from Brian Grunkemeyer.

http://blogs.msdn.com/brada/archive/2004/04/28/122423.aspx#123544

Ayende Rahien
07/28/2008 04:22 PM by
Ayende Rahien

Haacked,

The finalizer must run once, so it will still hold true

Cory Foy
07/28/2008 04:31 PM by
Cory Foy

But I still agree with Phil. In your example, you were making an assumption that the object would still be valid.

However, I think you are wrong in saying it will kill the app. It should only kill the thread. Of course, if your app is just on that one thread...

Here's an example. Here's my Class1:

public class Class1 : IDisposable

{

public System.IO.FileStream fs;


public Class1()

{


}


~Class1()

{

    fs.Dispose();

}


#region IDisposable Members


public void Dispose()

{

    fs.Dispose();

    GC.SuppressFinalize(this);

}


#endregion

}

And my Main:

class Program

{

static void Main(string[] args)

{

    Console.WriteLine("Hello, Class1!");

    System.Threading.Thread t= new System.Threading.Thread(

        new System.Threading.ThreadStart(DoIt));

    Console.WriteLine("Goodbye, World!");

    Console.ReadLine();

}


public static void DoIt()

{


    Class1 c1 = new Class1();

    Console.WriteLine("Goodbye, Class1");

    Console.ReadLine();

}

}

If you run this, you'll still see Goodbye, World, even though we are throwing an unhandled exception in the finalizer.

A better practice would be to call Dispose from your finalizer, and then put your checks in there to make sure that any object you are cleaning up is still around to be cleaned up. But you still make a good point to be aware of it.

Ayende Rahien
07/28/2008 05:22 PM by
Ayende Rahien

Cory,

You didn't start the thread :-)

You need to call GC.WaitForPendingFinalizers() as well. The finalizer will not run unless it has reason to.

Unhandled thread exception will kill the app

Cory Foy
07/28/2008 05:55 PM by
Cory Foy

Hahaha! Haha! Ha..Oh. [Smacks head]

It's interesting, because the docs I have say that if an unhandled exception occurs on the finalizer thread, then only the finalization of the object is terminated.

Oh wait! I know what I'm thinking of. I wonder if you are using Server GC versus Workstation GC if the behavior would be different. On workstation GC, the thread that does the allocation that triggers the GC does the GC, so if it dies, it takes the app down.

I have a dual proc here, so I'll have to try that out in a bit.

Cory Foy
07/28/2008 09:30 PM by
Cory Foy

I tried it out with Server GC, and it didn't make a difference. I've emailed some people to see if I'm just crazy.

Ayende Rahien
07/28/2008 10:15 PM by
Ayende Rahien

Cory,

That is the expected behavior, I would be surprised to learn otherwise

Patrik Akselsson
07/29/2008 08:10 AM by
Patrik Akselsson

A bit off topic, but do you really need to dispose the FileStream during finalization?

I was under the impression that you only need to make sure that unmanaged resources are released after finalization, whereas managed resources will be taken care of by the GC. The FileStream should release it's file handle during finalization, assuming that it is well behaved.

passing by
07/29/2008 09:55 AM by
passing by

Patrik Akselsson is right

In the finalizer you should NOT call fs.Dispose. The FileStream object (if it was instanciated, of course) will call its own finalizer to clean up.

You should look up the correct IDisposable pattern in MSDN.

I think simply removing the finalizer from your example makes your example work. But indeed, you can still have trouble if you try this:

BadClass bc;

try { bc = new BadClass(); }

finally { bc.Dispose(); }

(Note that the allocation is outside the try block if you use the "using" statement.)

That's why I always check for null in my Dipose method:

if (fs != null) { fs.Dispose(); fs = null; }

Changhong
07/29/2008 01:02 PM by
Changhong

Patrik, you are absolutely right. A finalizer should be use for releasing unmanaged resource only.

I remember a couple of years ago, I had a struggling with finalizer/Idisposeble, and I wanted to know how to implement a finalizer properly. But since I understood it, I never find a chance to implement it. So, I can hardly remember how to do it now. And I have seen quite a few times other developers implemented it, which none of them were necessary.

I found this post: http://ayende.com/Blog/archive/2008/07/28/Some-finalizer-pitfalls.aspx

from Joe Duffy tells me pretty much everything I want to know about finalizer/IDisposabe, and it does contain some comments on the Ayende’s issue:

“If a constructor throws an exception, the CLR will still call the object's Finalize method. So, when your Finalize method is called, the object's fields may not have all been initialized; your Finalize method should be robust enough to handle this.”

Changhong
07/29/2008 10:38 PM by
Changhong

oops, sorry about the wrong link.

Here is the right one:

http://www.bluebytesoftware.com/blog/2005/04/08/DGUpdateDisposeFinalizationAndResourceManagement.aspx

Konstantin Spirin
07/30/2008 04:15 PM by
Konstantin Spirin

AppDomain.CurrentDomain.UnhandledException += LogUnhandledException;

could significantly simplify your debugging.

wekempf
08/01/2008 06:45 PM by
wekempf

There's a reason you shouldn't touch a managed object in the finalizer. That object may have already been finalized, since the order in which objects are finalized is undefined. The suggested code that checks for null in the finalizer may work for you during development and testing, but crash and burn spectacularly when released into the wild. Follow the IDisposable pattern, and all will be well.

Comments have been closed on this topic.