Search code examples
c#nhibernategarbage-collectionidisposable

Dispose of objects references that live longer than the disposing instance


I am currently developing a unit of work and repository pattern on top of NHibernate (As a remark: I didn't made the decision for one or another pattern, therefore please don't discuss the usefulness of the repository pattern in regards to am ORM that already implements one). I first build per documentation a singleton (using IoC configuration) Sessionmanager that creates Units of Work instances (private methods removed for readability):

public sealed class SessionManager : ISessionManager
    {
        #region Members
        /// <summary>
        /// lock to manage thread safe access to the dictionary.
        /// </summary>
        private readonly ReaderWriterLockSlim _lock; 

        /// <summary>
        /// NHibernate sessionfactory to create sessions.
        /// </summary>
        private readonly ISessionFactory _factory;

        /// <summary>
        /// Units of Work that are already in use.
        /// </summary>
        private readonly Dictionary<string, IUnitOfWork> _units;

        /// <summary>
        /// Flag indicating if the manager got disposed.
        /// </summary>
        private bool _disposed;
        #endregion

        #region Constructors
        /// <summary>
        /// Creates a new Manager with the given Config.
        /// </summary>
        /// <param name="config"></param>
        public SessionManager(NHibernate.Cfg.Configuration config)
        {
            _lock = new ReaderWriterLockSlim();
            _units = new Dictionary<string, IUnitOfWork>();
            _factory = config
                .Configure()
                .AddAssembly(typeof(SessionManager).Assembly.FullName)
                .BuildSessionFactory();
            _disposed = false;
        }
        #endregion

        #region Methods
        /// <summary>
        /// Either creates or returns an existing unit of work.
        /// </summary>
        /// <param name="identifier">identifier of the uow</param>
        /// <param name="access">the data access that is needed for this uow.</param>
        /// <returns></returns>
        public IUnitOfWork Create(string identifier, DataAccess access)
        {
            Throw.IfNull(identifier, nameof(identifier));

            if (TryGetUnitOfWork(identifier, out var unit))
                return unit;

            return CreateOrReturn(identifier, access);
        }

        /// <summary>
        /// Disposes the given instance.
        /// </summary>
        /// <param name="unitOfWork">The unit of work that should get disposed.</param>
        public void DisposeUnitOfWork(IUnitOfWork unitOfWork)
        {
            Throw.IfNull(unitOfWork, nameof(unitOfWork));
            try
            {
                _lock.EnterWriteLock();
                if (_units.ContainsValue(unitOfWork))
                {
                    var key = _units.FirstOrDefault(x => x.Value.Equals(unitOfWork)).Key;
                    if (!string.IsNullOrWhiteSpace(key))
                        _units.Remove(key);
                }

                unitOfWork.Dispose();

            }
            finally
            {
                _lock.ExitWriteLock();
            }
            unitOfWork.Dispose();
        }

        /// <summary>
        /// Disposes managed and unmanaged ressources.
        /// </summary>
        /// <param name="disposing">Flag indicating if the call happened in the public <see cref="Dispose"/> method or the Finalizer.</param>
        void Dispose(bool disposing)
        {
            if (!_disposed)
            {
                if (disposing)
                {                   
                    foreach (var unit in _units)
                        unit.Value.Dispose();

                    _factory.Dispose();
                    _lock.Dispose();
                }

                // TODO: free unmanaged resources (unmanaged objects) and override a finalizer below.
                // TODO: set large fields to null.

                _disposed = true;
            }
        }

        /// <summary>
        /// Disposes managed and unmanaged ressources.
        /// </summary>
        public void Dispose()
        {
            Dispose(true);
            GC.SuppressFinalize(this);
        }
        #endregion
    }
}

The unit of work contains either a ISession or a IStatelessSession to create Transactions or the needed repositories.

public sealed class UnitOfWork : IUnitOfWork
    {
        #region Members
        private bool _disposed;

        public string Identifier { get; }

        public DataAccess Access { get; }

        private readonly ISession _session;

        private readonly IStatelessSession _statelessSession;
        #endregion

        #region Constructors
        private UnitOfWork(DataAccess access, string identifier)
        {
            Access = access;
            Identifier = identifier;
            _disposed = false;
        }

        internal UnitOfWork(DataAccess access, string identifier, ISession session)
            : this(access, identifier)
        {
            _session = session;
        }

        internal UnitOfWork(DataAccess access, string identifier, IStatelessSession session)
            : this(access, identifier)
        {
            _statelessSession = session;
        }
        #endregion

        #region Methods
        public void CloseTransaction(IGenericTransaction transaction)
        {
            transaction.Dispose();
        }


        public IGenericTransaction OpenTransaction()
        {
            if (_session != null)
                return new GenericTransaction(_session.BeginTransaction());

            if (_statelessSession != null)
                return new GenericTransaction(_statelessSession.BeginTransaction());

            throw new InvalidOperationException("You tried to create a transaction without having a vaild session.");
        }

        public IGenericTransaction OpenTransaction(IsolationLevel level)
        {
            if (_session != null)
                return new GenericTransaction(_session.BeginTransaction(level), level);

            if (_statelessSession != null)
                return new GenericTransaction(_statelessSession.BeginTransaction(level), level);

            throw new InvalidOperationException("You tried to create a transaction without having a vaild session.");
        }

        public bool Equals(IUnitOfWork other)
        {
            if (other == null)
                return false;

            return Access == other.Access;
        }

        public IRepository<T> GetRepository<T>() where T : Entity<T>
        {
            switch (Access)
            {
                case DataAccess.Statefull:
                    return new StatefullRepository<T>(_session);

                case DataAccess.Stateless:
                    return new StatelessRepository<T>(_statelessSession);

                default:
                    throw new ArgumentException("Cannot determine which repository is needed.", nameof(Access));
            }
        }

        #region IDisposable Support
        void Dispose(bool disposing)
        {
            if (!_disposed)
            {
                if (disposing)
                {
                    if (_session != null)
                        _session.Dispose();
                    if (_statelessSession != null)
                        _statelessSession.Dispose();
                }

                // TODO: free unmanaged resources (unmanaged objects) and override a finalizer below.
                // TODO: set large fields to null.

                _disposed = true;
            }
        }

        // TODO: override a finalizer only if Dispose(bool disposing) above has code to free unmanaged resources.
        // ~UnitOfWork() {
        //   // Do not change this code. Put cleanup code in Dispose(bool disposing) above.
        //   Dispose(false);
        // }

        // This code added to correctly implement the disposable pattern.
        public void Dispose()
        {
            // Do not change this code. Put cleanup code in Dispose(bool disposing) above.
            Dispose(true);
            // TODO: uncomment the following line if the finalizer is overridden above.
            // GC.SuppressFinalize(this);
        }
        #endregion
        #endregion
    }

As one can see the Repositories get created using either the ISession or the IStatelessSession. Both of these Interfaces implement the IDisposable interfaces meaning they should get disposed. Therefore my repositories implement IDisposable as well. Here however is the problem. In theory I can create as many repositories as I want from one unit of work e.g. :

public void UpdatePets(List<Cat> felines, List<Dog> carnines, ISessionManager manager)
        {
            var uow = manager.Create("Pets", DataAccess.Statefull);

            using (var t = uow.OpenTransaction())
            {
                using (var catRepo = uow.GetRepository<Cat>())
                {
                    catRepo.Add(felines);
                    t.Commit();
                }
            }

            //Some Businesslogic that forbids using one Transaction to mitigate the problem of an already disposed ISession or IStatelessSession.

            using(var t = uow.OpenTransaction())
            {
                using (var dogRepo = uow.GetRepository<Dog>())
                {
                    dogRepo.Add(carnines);
                    t.Commit();
                }
            }
        }

If that would be the service consuming my unit of work and I would use the "standard" IDisposable Pattern like this:

private void Dispose(bool disposing)
        {
            if (!_disposed)
            {
                if (disposing)
                {
                    if (_session != null)
                        _session.Dispose();
                }
                _disposed = true;
            }
        }

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

The second Repository would throw an ObjectDisposedexception because the ISession I reference in the second one is coming from my unit of work and that one was already disposed when my first repository left the using block. So to overcome this Problem I would do the following in my repository:

public void Dispose()
            {
                _session = null;
                GC.SuppressFinalize(this);
            }

This however feels wrong because instead of properly disposing the reference to an object that I should dispose I just "close my Eyes and forget about it" which is probably never to good of a solution in programming. So my question is basically: "Is there a good way to properly dispose objects that might live longer than the object holding the reference?"


Solution

  • You need to establish ownership, and use that to determine which class is responsible for cleaning each "thing" up. It's the owner and only the owner that should call Dispose, no matter how many other classes may have interacted with it.

    Ownership needs to be exclusive and (except for wrapper classes) nested1 - the lifetime of the owner needs to be at least as long as the owned object. It's by following these two rules that we can ensure that Dispose is a) called once2 b) when the object is no longer required.

    The repositories are the wrong thing to "own" the session. Multiple repositories can be given the same session (so if they're the owners, we've lost the exclusive property), and as your own title alludes to, they have a shorter lifetime than it.


    Some general rules of thumb

    • an object passed as a parameter to an instance method will not normally become owned by the instance.
    • an object created by an instance and that lives longer than the lifetime of a single method call will be owned by the instance
    • an object created by an instance method and that doesn't live longer than the lifetime of a single method call will be owned by the method

    1For wrappers, the inner object will tend to be created first, possibly configured, then passed to the wrapper object. So the wrapper object's lifetime starts after the inner object.

    2Not strictly required. Disposables should tolerate Dispose being called multiple times, but that's more for belt-and-braces defensiveness than a desired pattern.