Why is calling Dispose() in a finalizer causing an ObjectDisposedException?

I have a NHibernate repository that looks like this:

public class NHibRepository : IDisposable
{
    public ISession Session { get; set; }
    public ITransaction Transaction { get; set; }

    // constructor
  开发者_如何学运维  public NHibRepository()
    {
        Session = Database.OpenSession();
    }

    public IQueryable<T> GetAll<T>()
    {
        Transaction = Session.BeginTransaction();
        return Session.Query<T>();
    }

    public void Dispose()
    {
        if (Transaction != null && Transaction.IsActive)
        {
            Transaction.Rollback(); // ObjectDisposedException on this line
        }

        Session.Close();
        Session.Dispose();
    }

    ~NHibRepository()
    {
        Dispose();
    }
}

When I use the repository like this, it runs fine:

using (var repo = new NHibRepository())
{
    Console.WriteLine(repo.GetAll<Product>().Count());
}

But when I use it like this, it will throw an ObjectDisposedException:

var repo = new NHibRepository();
Console.WriteLine(repo.GetAll<Product>().Count());

The easy solution would be to always dipose of the repository explicitly, but unfortunately I don't control the life cycle of some of the classes that use the repository.

My question is, why is the Transaction disposed of already even though I did not explicitly call Dispose()? I'd like to have the repository automatically clean itself up if it was not disposed explicitly.

My question is, why is the Transaction disposed of already even though I did not explicitly call Dispose()?

Perhaps the transaction's finalizer ran first. Remember, finalizers of dead objects can run in any order and on any thread, and need not have been correctly initialized before they are finalized. If you do not understand all the rules of finalizers then learn them all before you attempt to write any more code that uses finalizers. This is one of the hardest things to get right.

It also looks as though you have implemented the disposable pattern incorrectly, and that is going to cause you a world of grief. Read up on the pattern and do it correctly; the finalizer should not be disposing stuff that has already been disposed:

http://msdn.microsoft.com/en-us/magazine/cc163392.aspx

Lose the finalizer. Very few classes in .net need a finalizer; generally, the only .net 2.0-or-later classes which should have finalizers are those whose sole reason for existence revolves around it.

If a class with a finalizer holds an access to some other object, one of three conditions will apply when the finalizer is run:

1. The other object has already had its finalizer (if any) run; there's no need to dispose it, since it's already been taken care of.
2. The other object has a finalizer scheduled to run; there's no need to dispose it, since it will be taken care of automatically.
3. A reference to the other object exists outside of the object whose finalizer is running; this usually means one shouldn't dispose it.

The only time a finalizer should ever take any action to dispose of a manage object is when an outside reference is likely to exist, and the collection of the object being finalized will imply that the other object should be disposed despite the existence of that reference. That's a very rare situation.

You should put GC.SuppressFinalize(this); in your Dispose method otherwise the finalizer will dispose an already disposed object. Also, finalizers are in almost all cases only needed for unmanaged resources

The CLR makes no guarantees with respect to the order that finalizers will be called. It sees a group of unrooted objects (e.g. not reachable from any GC root) and starts calling finalizers. It doesn't matter that you have connections within your object graph. The finalizers can be called in any order. Finalizers are intended to clean up unmanaged resources, not child objects. You need to re-think your API.

You should have a member variable isDisposed that would be set to true by the Dispose() method. Then at the beginning of the Dispose() command just return if it is already set to true. According to .NET whitepapers, Dispose() could be executed multiple times without side effects and this is the way to do it.

Secondly, make the class sealed (the class is not implementing the Dispose pattern properly for inheritance) and put GC.SuppressFinalize(this); right before the isDisposed variable assignment in the Dispose() method.

public void Dispose()
{
   if (isDisposed) { return; }

   ....

   GC.SuppressFinalize(this);   
   isDisposed = true;
}