Finalizer vs. Application: A Race Condition from Hell

July 28, 2008

5 comments

One of my favorite managed debugging demos is analyzing a memory leak caused by a blocking finalizer.  This tension between the finalizer thread and the application threads making allocations should be kept in mind by any programmer using this powerful but dangerous feature.  However, there is another subtle category of bugs that can surface “thanks” to finalization: Race conditions between the finalizer and your application threads.

For example, consider the following scenario in which the File class acts as a wrapper over an implementation-provided Handle type:

class File : IDisposable

{

    Handle h;

 

    public File(string fn)

    {

        h = new Handle(fn);

    }

 

    public void Read()

    {

        Util.InternalRead(h);

    }

 

    ~File()

    {

        h.Close();

    }

 

    public void Dispose()

    {

        h.Close();

        GC.SuppressFinalize(this);

    }

}

 

class Program

{

    static void Main(string[] args)

    {

        File f = new File(“1.txt”);

        f.Read();

    }

}

The class’ code seems perfectly solid even to the seasoned reviewer (although utterly useless :-) ).  It provides deterministic finalization through IDisposable, it provides a backup through a non-deterministic finalizer, it is very cautious to close the handle only once…  The client code is a bit flaky as it doesn’t call Dispose (nor uses the using statement), but the finalizer should take care of things.  So what am I hinting at?

image

Due to the GC’s eager nature, it’s entirely possible for the file handle to be closed in the middle of the InternalRead operation.  This is a very disturbing consequence that can be the source of extremely difficult debugging scenarios.

Once you have overcome the shock, let’s follow the reasoning:  The object itself is considered in use as long as it is referenced.  Once it is no longer referenced, it becomes eligible for garbage collection and subsequently for finalization as well.  In our case, the initial references are the local variable f in the main method, and the this implicit parameter (which is normally kept in the ECX register) in the Read method.  The f local variable is not used after the f.Read() call, so it does not prevent the object from being collected.  The surprise stems from the fact that the this parameter is not used after the file handle is passed to the InternalRead method, and therefore doesn’t prevent the object from being collected either!  Closing the handle in the middle of the InternalRead call is a bug from hell, and it’s non-deterministic by definition because we have no idea when exactly the finalizer will be called.

Oh, and have I mentioned it?  The bug will surface only in the Release build, because in the Debug build the GC treats local variables as active roots until the end of their enclosing scope to facilitate debugging.  Did you need more motivation for running your tests against Release build as well?…

Add comment
facebook linkedin twitter email

Leave a Reply

Your email address will not be published.

*

You may use these HTML tags and attributes: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <strike> <strong>

5 comments

  1. David LevineJuly 28, 2008 ב 9:22 PM

    Sasha,

    I blogged about a similar subject several years ago here: http://blogs.msdn.com/ricom/archive/2004/05/19/135332.aspx

    It’s possible to make the constructor, let alone an instance method, run at the same time as the finalizer.

    This kind of bug is very hard to catch before it ships. Even a good code review is unlikely to catch it, and most automated testing wont detect it either as it is essentially random.

    However, there are techniques that can be used to detect part of the problem, and it’s one I outlined in the blog.

    1. IDisposable should be treated as a contract requirement by all users of the class. Failure to call Dispose is tantamount to violating the contract.

    2. All classes that implement IDisposable should also implement a finalizer as a backstop, unless there are strong performance reasons to not do so.

    However, the invocation of the finalizer means that a client violated the contract! Since this “should” be a rare occurrence, the class could leave breadcrumbs behind (e.g. generate a trace message) to indicate a usage problem occurred.

    I agree that the release build should be used for testing. This makes it more likely that the problem surfaces before it ships.

    Reply
  2. DmitriyJuly 29, 2008 ב 3:45 AM

    Does it solves the problem ?

    public void Read()
    {
    Util.InternalRead(h);
    GC.KeepAlive(this);
    }

    Reply
  3. BillyJuly 29, 2008 ב 9:53 AM

    Should there not still be a ‘this’ pointer for f on the stack until the function returns? I thought that the GC included all of the stack variables?

    Reply
  4. SkupJuly 30, 2008 ב 8:43 AM

    Actually you should never access reference type in finalizers, because it could have been finalized already.
    But that is not a problem since the file has also a finalizer that will dispose its unmanaged resources.

    This is why the recommended parter for IDisposable is the folowing :

    class DisposableObject : IDisposable
    {

    ~DisposableObject()
    {
    Dispose(false);
    }

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

    protected virtual void Dispose(bool disposing)
    {
    if (disposing)
    {
    // dispose managed objects
    }

    // dispose unmanaged resources
    }

    }

    Reply
  5. Sasha GoldshteinJuly 30, 2008 ב 2:49 PM

    @David: Agreed, a finalizer should be used as a backstop. I sometimes put Debug.Assert or Trace.Assert in the finalizer in these scenarios during the testing phase. Unfortunately, contractual guarantees are often not met, and many people have accumulated lots of grief with regard to .NET’s approach, which is either contractual and deterministic *or* (almost) guaranteed but non-deterministic. They forget that native code doesn’t have any stronger guarantees… :-)

    @Dmitriy: Yes, GC.KeepAlive(this) will do the trick.

    @Billy: In the Release build, the GC considers local variables (and registers) as long as they are in use by the method (on an IP basis). Therefore, ‘f’ is no longer relevant after the instruction that dispatches the call.

    Reply