Not Wrapping IDisposable Objects in Using Statements – C# Best Practices

asp.netasp.net-mvccdependency-injectionunit testing

I am updating some code to allow us to implement unit tests. What I have so far is a Business Layer which calls the repository to get data from the database.

Business Layer example:

public class ConversationLogic
{
    private IConversationData _conversationData { get; set; }

    public ConversationLogic(IConversationData conversationData)
    {
        _conversationData = conversationData;
    }

    public ConversationListModel GetConversations(int loginID)
    {
        ConversationListModel model = new ConversationListModel();

        dynamic list = _conversationData.GetConversations(loginID);

        foreach (var item in list)
        {
            ConversationModel conversation = ConvertConversation(item);

            model.Conversations.Add(conversation);
        }

        return model;
    }
}

This allows an Interface of ConversationData to be passed in, this ConversationData is the repository layer which calls the database.

Conversation Data repository example:

public class ConversationData : BaseMassiveTable, IConversationData
{
    public dynamic GetConversations(int loginID)
    {
        string sql = GetResourceFile("Project.SQL.GetConversations.sql", Assembly.GetExecutingAssembly());
        dynamic result = Query(sql, loginID).ToList();
        return result;
    }
}

The ConversationData inherits from the class BaseMassive, which implements IDisposable:

public class BaseMassiveTable : DynamicModel, IDisposable
{
    string connectionName;

    public BaseMassiveTable(string connectionStringName, string tableName, string primaryKeyColumn) :
        base(connectionStringName, tableName, primaryKeyColumn) 
    { 
        connectionName = connectionStringName;
    }

    public void Dispose()
    {
    }

    protected void WriteErrorToFile(int userId, string url, Exception ex)
    {
        try
        {
            string path = "~/Error/" + DateTime.Today.ToString("yyyy-MM-dd") + ".txt";
            if (!File.Exists(System.Web.HttpContext.Current.Server.MapPath(path)))
            {
                File.Create(System.Web.HttpContext.Current.Server.MapPath(path)).Close();
            }
            using (StreamWriter w = File.AppendText(System.Web.HttpContext.Current.Server.MapPath(path)))
            {
                w.WriteLine("\r\nLog Entry : ");
                w.WriteLine("{0}", DateTime.Now.ToString(CultureInfo.InvariantCulture));
                string err = "Error in: " + url +
                              ". Error Message:" + ex.Message;
                w.WriteLine(err);
                w.WriteLine(ex.StackTrace.ToString());
                w.WriteLine("__________________________");
                w.Flush();
                w.Close();
            }
        }
        catch
        {
            //What do we do here ????
        }
    }

    public string Truncate( string stringForTuncation, int maxLength)
    {
        string result = "";
        if (stringForTuncation == null)
        {
            result = null;
        }
        else if (stringForTuncation.Length <= maxLength)
        {
            result = stringForTuncation;
        }
        else
        {
            result = stringForTuncation.Substring(0, maxLength);
        }
        return result;
    }

    public static string GetResourceFile(string name, Assembly assembly)
    {
        using (Stream stream = assembly.GetManifestResourceStream(name))
        {
            using (StreamReader reader = new StreamReader(stream))
            {
                string result = reader.ReadToEnd();
                return result;
            }
        }
    }

}

As you may seen in my implementation of ConversationLogic I cannot wrap the IConversationData interface in a using as this does not work with the current implementation.

What you will also see that in ConversationData.GetConversations(), this calls the Massive ORM Query(), which looks like this:

public virtual IEnumerable<dynamic> Query(string sql, params object[] args)
{
    using (var conn = OpenConnection())
    {
        var rdr = CreateCommand(sql, conn, args).ExecuteReader();
        while (rdr.Read())
        {
            yield return rdr.RecordToExpando(); ;
        }
    }
}

What I am looking for is that, even though ConversationData inherits from BaseMassive, which implements IDisposable, do I actually need to wrap any calls using IConversationData in using statements, as every call to the repository calls the Massive Query() statement, which manages its own resources by creating a using() statement on any query?

Apologies for the long-winded question, if you need any clarifications, just let me know.

Best Answer

The caller should use using; you need to implement Dispose in order for him to be able to. Then the Dispose calls will cascade and everything will get released in due course.

The class into which the disposable resource is injected should have its own Dispose method and should release the resources there. Then you need to make sure that whoever is using the ConversationLogic class is also disposing it, perhaps with a using clause of their own.

For example, if your ConversationLogic class is injected into an MVC controller, you can take advantage of the fact that the base Controller class already has a Dispose method. You just need to override it to Dispose your ConversationLogic, which will in turn release your resource-intensive service. The .NET pipeline will call the controller's Dispose method at the right time.

class MyController : System.Web.Mvc.Controller
{
    private readonly IConversationLogic _conversationLogic;

    public MyController(IConversationLogic conversationLogic)
    { 
        _conversationLogic = conversationLogic;
    }

    public override void Dispose(bool disposing)
    {
        if (disposing)
        {
            if (_conversationLogic != null)
            {
                _conversationLogic.Dispose(true);
                _conversationLogic = null;
            }
            base.Dispose(true);
        }
    }
}

class ConversationLogic : IConversationLogic, IDisposable
{
    private readonly _conversationData;

    public ConversationLogic(IConversationData conversationData)
    {
        _conversationData = conversationData;
    }

    public override Dispose(bool disposing)
    {
        if (disposing)
        {
            if (_conversationData != null)
            {
                _conversationData.Dispose();
                _conversationData = null;
            }
        }
        base.Dispose(disposing);
    }
}

Sometimes you're not sure if the injected resource implements IDisposable. It is easy to check.

    public override Dispose(bool disposing)
    {
        if (disposing)
        {
            if (_conversationData != null)
            {
                var d = _conversationData as IDisposable;
                if (d != null) 
                {
                    d.Dispose();
                }
                _conversationData = null;
            }
        }
        base.Dispose(disposing);
    }

If this pattern bothers you (because you're a purist, and you noticed that the code that created the resource didn't dispose it) you have two options:

  1. Get over it, because the code that created the resource was the IoC container, and it would require something really weird for the IoC container to deal with the disposal, or

  2. Don't inject the disposable instance; inject a factory instead. That way your class can both create and dispose of the object and make everyone happy.

    class ConversationLogic: IConversationLogic, IDisposable
{
    private readonly _conversationData;

    public ConversationLogic(IConversationDataFactory conversationDataFactory)
    {
        _conversationData = conversationDataFactory.GetInstance();
    }

    public override Dispose(bool disposing)
    {
        if (disposing)
        {
            if (_conversationData != null)
            {
                _conversationData.Dispose();
                _conversationData = null;
            }
        }
        base.Dispose(disposing);
    }
}

Either way, you don't use using, you use the Dispose override per the conventional pattern, and ensure that your controller (or other object) is itself enclosed in a using clause so that Dispose gets called.

Related Topic