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();

}

}

Haacked,

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

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

Haacked,

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

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.

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

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.

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.

Cory,

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

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.

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; }

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.”

oops, sorry about the wrong link.

Here is the right one:

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

AppDomain.CurrentDomain.UnhandledException += LogUnhandledException;

could significantly simplify your debugging.

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.