Does this implementation of the Entity Framework leaks memory?

Question

I just can't make out if the entity context is disposed in the usage flow when used in a using statement in a web application or a console application.

Thanks!

using System;
using System.Web;

namespace Foo.Model
{
    public partial class FooEntities : ObjectContext
    {
        private const string CurrentContextKey = "FooEntities.Current";

        [ThreadStatic]
        private static FooEntities _currentOnThreadStatic;
        private FooEntities _previousContext;

        /// <summary>
        /// Gets the current <see cref="FooEntities"/> instance, if an instance can be shared in the current context.
        /// </summary>
        /// <remarks>
        /// The current context is stored in the HTTP context, if it is available (otherwise it is stored in a thread-static instance).
        /// Multiple contexts can be stacked.
        /// </remarks>
        public static FooEntities Current
        {
            get
            {
                if (HttpContext.Current != null)
                {
                    return HttpContext.Current.Items[CurrentContextKey] as FooEntities;
                }
                else
                {
                    return _currentOnThreadStatic;
                }
            }

            private set
            {
                if (HttpContext.Current != null)
                {
                    HttpContext.Current.Items[CurrentContextKey] = value;
                }
                else
                {
                    _currentOnThreadStatic = value;
                }
            }
        }

        /// <summary>
        /// Returns a repository instance bound to this object context.
        /// </summary>
        /// <typeparam name="TRepository">The type of repository to instantiate.</typeparam>
        /// <returns>The repository instance.</returns>
        public TRepository GetRepository<TRepository>()
            where TRepository: BaseRepository
        {
            return (TRepository) Activator.CreateInstance(typeof(TRepository), this);
        }

        /// <summary>
        /// Ensures that an ambient context is available through <see cref="Current"/>, throwing an exception otherwise.
        /// </summary>
        /// <exception type="InvalidOperationException)">
        /// Thrown if <see cref="Current"/> is null.
        /// </exception>
        public static void EnsureContext()
        {
            if (Current == null)
            {
                throw new InvalidOperationException("An ambient FooEntities context is expected.");
            }
        }

        /// <summary>
        /// Releases the context instance.
        /// </summary>
        /// <param name="disposing"></param>
        protected override void Dispose(bool disposing)
        {
            Current = _previousContext;
            base.Dispose(disposing);
        }

        /// <summary>
        /// Is called by all constructors.
        /// </summary>
        partial void OnContextCreated()
        {
            _previousContext = Current;
            Current = this;
        }
    }
}

Arleen

Do you have a reason for trying to store an ObjectContext inside your HttpContext? A context is considered light-weight, typically you create an instance when you need it, then dispose of it. – Zeralda

So FooEntities.Current may be used to in diffident classes so you don't have to inject an instance in the constructor and keep the same instance across all those classes. – Arleen

I think you should read this one: #3653509 That would hopefully push you to reconsider that thread static context. – Liquid

@Ladislav Mrnka: I think the implementation id c*&p. I don't support it, nor did I write it, nor imagine it. I believe that the entity context should be injected into the classes that need to access the model and the top level request handler should instantiate it. It is ugly ugly ugly! Bottom line I agree with you and hope no one takes it as an example of what to do but rather what not to do. – Arleen

Answer 1

It is an odd design. As @Joel C points out in his comment you should regard the object context as a shortlived object that you create when you need it and release right afterwards.

But I see no reason that this would leak memory. You are only dealing with managed resources and you are using the same key all the time to the HttpContext so you won't create new objects all over.

Carliecarlile

ObjectContext is IDisposable, so it's not a managed resource. That's why I worry about memory leaks. – Arleen

Have a look at the OnContextCreated method which is called in the constructor of the other partial of the class. – Arleen

@Four: Even if it is IDisposable it will be correctly freed by the garbage collector. IDisposable is an option (that should be used) to release expensive resources early, instead of waiting for the GC, but you won't introduce a leak if you ignore it. – Carliecarlile

@Four: Sorry, didn't see the OnContextCreated before, but I still think it's fine because the "anchor" keeping the linked list alive is HttpContext which itself is very short lived. – Carliecarlile

@Anders: The primary use of this interface is to release unmanaged resources. ... Furthermore, the garbage collector has no knowledge of unmanaged resources such as window handles, or open files and streams. msdn.microsoft.com/en-us/library/system.idisposable.aspx – Arleen

It's more like a memory "balloon" than a memory leak. Everything will eventually get freed, so it's not technically a leak. But the more entities that get loaded into the context before it's eventually disposed, the more memory will be taken up for change tracking, tracking relations, etc. – Zeralda

In a correct implementation of IDisposable, the object finalizer ensures that any unmanaged resources are released. The finalizer is automatically called by the GC, so no, you can safely ignore IDisposable. However it is best practice to dispose of them instead of waiting for the GC. – Carliecarlile

@Anders not really, the finalizer ensures managed resources are released. If you're using unmanaged resources such as handles, you're responsible for doing that yourself, and not disposing of your object properly will cause memory leaks. But it's most commonly used with managed resources to ensure that things are disposed of in a timely manner instead of waiting for GC, such as for database connections. – Zeralda

@Anders and @ Joel: Since the HttpRequest is shot lived and it was my feeling too that it looked more like a memory balloon than a leak. It answers my question. Thanks guys. By the way It's not my code... (had to mention it) – Arleen

@Joel: If the ordinary pattern for implementing IDiposable is followed, the finalizer calls Dispose(false) which should release any unmanaged resources. The false flag tells Dispose(bool disposing) that it should ignore any managed references and only release unmanaged resources. – Carliecarlile

This discussion made me ask a question where the details are sorted out. – Carliecarlile