'using' keyword vs class field for Entity Framework

Question

I know the using keyword basically calls Dispose() after scope ends, however, is it bad practice or unacceptable to have the variable from the using clause be a class field? Example:

public class ValuesController : ApiController
{
        DatabaseEntities db;

        public ValuesController()
        {
            db = new DatabaseEntities();
        }

        public IHttpActionResult GetList()
        {
            return Ok(db.Values.ToList());
        }
}

vs:

public class ValuesController : ApiController
{     
        public IHttpActionResult GetList()
        {
            using (DatabaseEntities db = new DatabaseEntities())
            {
                 return Ok(db.Values.ToList());
            }
        }
}

Will the DatabaseEntities properly dispose after the API call is complete in the first example? Is the first example bad practice?

Answer 1

It is safe to encapsulate it in your Controller, but only if you Dispose it in the Controller's Dispose(bool). eg:

public class ValuesController : ApiController
{
    DatabaseEntities db;

    public ValuesController()
    {
        db = new DatabaseEntities();
    }

    public IHttpActionResult GetList()
    {
        return Ok(db.Values.ToList());
    }
    protected override void Dispose(bool disposing)
    {
        if (disposing)
        {
            db.Dispose();
        }
        base.Dispose(disposing);
    }
}

But using DI is easier than overriding Dispose(bool) on your controllers, so it's probably not a best-practice.

Answer 2

The first example is bad practice on two counts

1. You should inject your dependencies into the controller using a DI library

2. If your DatabaseEntities class uses non-managed resources (like say some COM objects built on Win32, etc) then it will not be disposed of by the GC properly; the class itself might have a finaliser method it calls when it goes out of scope but in general it's not a good idea to rely on these being present. The net effect of this can be both memory leaks and loss of service in some cases (like if the underlying COM server has a limited pool of objects, or if a finaliser crashes, etc).

There's a great discussion about that here

And, even if it is is a fully managed class, it will not be disposed of until the next GC cycle in your scenario, net result being performance issues.

So your code should look like this (note, here I'm using an interface which is also considered good practice in terms of abstracting the actual implementation of the dependency).

public class ValuesController : ApiController
{
        readonly IDatabaseEntities _db;

        //here db is being provided to the ctor by your
        //DI Library e.g. StructureMap, SimpleInjector, .Net Core Injection, etc.
        public ValuesController(IDatabaseEntities db)
        {
            _db = db; //null check left out for brevity
        }

        public IHttpActionResult GetList()
        {
            return Ok(_db.Values.ToList());
        }
}

The beauty of this is that your DI container is now responsible for disposing of your DatabaseEntities instances, and not you. And it makes your ValuesController easier to test in isolation as well.

Answer 3

I have 2 rules about Disposeables that have never failed me:

1. Never split up the creation and disposing. Create, Use, Dispose. All in teh same piece of code, ideally using a using block.
2. There are cases when you can not follow Rule 1. Mostly because your class contains something Disposeable, that needs to be kept open. In that case you implement IDisposeable for the sole purpose of relaying the dispose call. Indeed that is about the reason 95% of all classes do implement IDisposeable - because they may have to do that kind of relay.

In case 2 whoever uses your class follows rule 1.

Making a class field of something Disposeable except a case 2, will really only risk stuff like "trying to work with it when already disposed" and "trying to do stuff with it, when nobody should be doing stuff with it". It will just cause issues.

Edit:

Based on your comments, you are mostly worried about the polling application side. If you do polling, there is a very common pattern to have teh whole polling work - loop and all - run in an alternative task or thread. This is my rate-limit code for multithreading, modified with a using:

integer interval = 20;
DateTime dueTime = DateTime.Now.AddMillisconds(interval);

using(someThingDisposeable){
  while(true){
    if(DateTime.Now >= dueTime){
      //insert your work with someThingDisposeable here

      //Update next dueTime
      dueTime = DateTime.Now.AddMillisconds(interval);
    }
    else{
      //Just yield to not tax out the CPU
      Thread.Sleep(1);
    }
  }
}

Modifying it for other forms of Multitasking, only requires using their equivalent for Thread.Sleep.

This code will keep the connection open as long as everything goes well. It will not query the datasource more often then you tell it too. It will never stack up operations. But it will still reliably close the connection on Exceptions. Do not forget to expose the Exception - with Multitasking it is notoriously easy to forget that part. It is a full case 1, minus needing to re-create the connection over and over.