Finalizers and Dispose

Question

I've got a class named BackgroundWorker that has a thread constantly running. To turn this thread off, an instance variable named stop to needs to be true.

To make sure the thread is freed when the class is done being used, I've added IDisposable and a finalizer that invokes Dispose(). Assuming that stop = true does indeed cause this thread to exit, is this sippet correct? It's fine to invoke Dispose from a finalizer, right?

Finalizers should always call Dispose if the object inherits IDisposable, right?

/// <summary>
/// Force the background thread to exit.
/// </summary>
public void Dispose()
{
    lock (this.locker)
    {
        this.stop = true;
    }
}

~BackgroundWorker()
{
    this.Dispose();
}

Answer 1

Your code is fine, although locking in a finalizer is somewhat "scary" and I would avoid it - if you get a deadlock... I am not 100% certain what would happen but it would not be good. However, if you are safe this should not be a problem. Mostly. The internals of garbage collection are painful and I hope you never have to see them ;)

As Marc Gravell points out, a volatile bool would allow you to get rid of the lock, which would mitigate this issue. Implement this change if you can.

nedruod's code puts the assignment inside the if (disposing) check, which is completely wrong - the thread is an unmanaged resource and must be stopped even if not explicitly disposing. Your code is fine, I am just pointing out that you should not take the advice given in that code snippet.

Yes, you almost always should call Dispose() from the finalizer if implementing the IDisposable pattern. The full IDisposable pattern is a bit bigger than what you have but you do not always need it - it merely provides two extra possibilities:

1. detecting whether Dispose() was called or the finalizer is executing (you are not allowed to touch any managed resources in the finalizer, outside of the object being finalized);
2. enabling subclasses to override the Dispose() method.