Does ADO.NET go overboard with IDisposable?

Question

When .NET first came out, I was one of many who complained about .NET's lack of deterministic finalization (class destructors being called on an unpredictable schedule). The compromise Microsoft came up with at the time was the using statement.

Although not perfect, I think using using is important to ensuring that unmanaged resources are cleaned up in a timely manner.

However, I'm writing some ADO.NET code and noticed that almost every class implements IDisposable. That leads to code that looks like this.

using (SqlConnection connection = new SqlConnection(connectionString))
using (SqlCommand command = new SqlCommand(query, connection))
using (SqlDataAdapter adapter = new SqlDataAdapter(command))
using (SqlCommandBuilder builder = new SqlCommandBuilder(adapter))
using (DataSet dataset = new DataSet())
{
    command.Parameters.AddWithValue("@FirstValue", 2);
    command.Parameters.AddWithValue("@SecondValue", 3);

    adapter.Fill(dataset);

    DataTable table = dataset.Tables[0];
    foreach (DataRow row in table.Rows) // search whole table
    {
        if ((int)row["Id"] == 4)
        {
            row["Value2"] = 12345;
        }
        else if ((int)row["Id"] == 5)
        {
            row.Delete();
        }
    }
    adapter.Update(table);
}

I strongly suspect I do not need all of those using statements. But without understanding the code for each class in some detail, it's hard to be certain which ones I can leave out. The results is kind of ugly and detracts from the main logic in my code.

Does anyone know why all these classes need to implement IDisposable? (Microsoft has many code examples online that don't worry about disposing many of these objects.) Are other developers writing using statements for all of them? And, if not, how do you decide which ones can do without?

Answer 1

One part of the problem here is that ADO.NET is an abstract provider model. We don't know what a specific implementation (a specific ADO.NET provider) will need with regards to disposal. Sure, we can reasonably assume the connection and transaction need to be disposed, but the command? Maybe. Reader? Probably, not least because one of the command-flags options allows you to associate a connection's lifetime to the reader (so the connection closes when the reader does, which should logically extend to disposal).

So overall, I think it is probably fine.

Most of the time, people aren't messing with ADO.NET by hand, and any ORM tools (or micro-ORM tools, such as "Dapper") will get this right for you without you having to worry about it.

---

I will openly confess that on the few occasions when I've used DataTable (seriously, it is 2018 - that shouldn't be your default model for representing data, except for some niche scenarios): I haven't disposed them. That one kinda makes no sense :)

Answer 2

If a class implements IDisposable, then you should always make sure that it gets disposed, without making any assumptions about its implementation.

I think that's the minimal amount of code that you could possibly write that ensures that your objects will be disposed. I see absolutely no problem with it, and it does not distract me at all from the main logic of my code.

If reducing this code is of paramount importance to you, then you can come up with your own replacement (wrapper) of SqlConnection whose child classes do not implement IDisposable and instead get automatically destroyed by the connection once the connection is closed. However, that would be a huge amount of work, and you would lose some precision with regards to when something gets disposed.

Answer 3

Yes. almost anything in ADO.Net is implementing IDisposable, whether that's actually needed - in case of, say, SqlConnection(because of connection pooling) or not - in case of, say, DataTable (Should I Dispose() DataSet and DataTable? ).

The problem is, as noted in the question:

without understanding the code for each class in some detail, it's hard to be certain which ones I can leave out.

I think that this alone is a good enough reason to keep everything inside a using statement - in one word: Encapsulation.

In many words: It shouldn't be necessary to get intimately familiar, or even remotely familiar for that matter, with the implementation of every class you are working with. You only need to know the surface area - meaning public methods, properties, events, indexers (and fields, if that class has public fields). from the user of a class point of view, anything other then it's public surface area is an implementation detail.

Regarding all the using statements in your code - You can write them only once by creating a method that will accept an SQL statement, an Action<DataSet> and a params array of parameters. Something like this:

void DoStuffWithDataTable(string query, Action<DataTable> action, params SqlParameter[] parameters)
{
    using (SqlConnection connection = new SqlConnection(connectionString))
    using (SqlCommand command = new SqlCommand(query, connection))
    using (SqlDataAdapter adapter = new SqlDataAdapter(command))
    using (SqlCommandBuilder builder = new SqlCommandBuilder(adapter))
    using (var table = new DataTable())
    {
        foreach(var param in parameters)
        {
            command.Parameters.Add(param);
        }
        // SqlDataAdapter has a fill overload that only needs a data table
        adapter.Fill(table);
        action();
        adapter.Update(table);
    }
}

And you use it like that, for all the actions you need to do with your data table:

DoStuffWithDataTable(
    "Select...",
    table => 
    { // of course, that doesn't have to be a lambda expression here...
        foreach (DataRow row in table.Rows) // search whole table
        {
            if ((int)row["Id"] == 4)
            {
                row["Value2"] = 12345;
            }
            else if ((int)row["Id"] == 5)
            {
                row.Delete();
            }
        }
    },
    new SqlParameter[]
    {
        new SqlParameter("@FirstValue", 2),
        new SqlParameter("@SecondValue", 3)
    }
    );

This way your code is "safe" with regards of disposing any IDisposable, while you have only written the plumbing code for that just once.