On using "using" and "finally" to cleanup resources

Question

Is there any case in which the following structure is needed?

using (Something something = new Something())
{
    try
    {
    }
    finally
    {
        something.SomeCleanup();
    }
}

Or, should all cleanup tasks be performed in the implicit something.Dispose() call?

---

Here is the offending code:

public static DataTable GetDataTable(string cmdText, IEnumerable<Parameter> parameters)
{
    // Create an empty memory table.
    DataTable dataTable = new DataTable();

    // Open a connection to the database.
    using (SqlConnection connection = new SqlConnection(ConfigurationTool.ConnectionString))
    {
        connection.Open();

        // Specify the stored procedure call and its parameters.
        using (SqlCommand command = new SqlCommand(cmdText, connection))
        {
            command.CommandType = CommandType.StoredProcedure;

            SqlParameterCollection parameterCollection = command.Parameters;
            foreach (Parameter parameter in parameters)
                parameterCollection.Add(parameter.SqlParameter);

            try
            {
                // Execute the stored procedure and retrieve the results in the table.
                using (SqlDataAdapter dataAdapter = new SqlDataAdapter(command))
                    try
                    {
                        dataAdapter.Fill(dataTable);
                    }
                    catch
                    {
                        dataTable.Dispose();
                        dataTable = null;
                    }
            }
            finally
            {
                //parameterCollection.Clear();
            }
        }
    }

    return dataTable;
}

NOTE: I have defined the Parameter class so users of this function don't have to deal with the creation of SqlParameters directly. The SqlParameter property of the Parameter class can be used to retrieve an SqlParameter.

At some point, my program does the following (cannot post the code, because it involves a lot of classes; basically, I have a mini-framework creating lots of objects):

1. Create an array of Parameters.
2. GetDataTable('sp_one', parameters).
3. GetDataTable('sp_two', parameters).

Answer 1

The using keyword only calls the .Dispose() method. If you have necessary clean up that happens outside of the dispose method on an IDisposable object, then you will need to do that in it's own finally block. This bring up two points:

1. At this point, you could argue that you might as well skip the using block and just call Dispose() inside the finally block, too. Personally, I'd still go with a using block. It's just a good habit to be in to always have one for your IDisposable instances.
2. I humbly suggest that if you meed the conditions above, you need to re-design your class to take advantage of the IDisposable pattern.

---

Based on the code you posted, the problem is that your parameters are still rooted somewhere (perhaps you're re-using them?). Because the parameters are still rooted they cannot be collected. They also contain a reference to the command they are attached to, so your SqlCommand object cannot be collected right away either, because now it's still rooted as well.

The key point is that the .Net framework reserves the Dispose() pattern for unamanaged resources. Because SqlParameters and SqlParameterCollection are managed types, they are not touched until they are collected, which happens completely separately from disposal. When your SqlCommand is finally collected, it's SqlParameter collection will be taken care of as well. Just don't confuse collection, disposal, and their purposes.

What you want to do is make a copy of each parameter when you add it, rather than add the existing parameter to the collection.

public static DataTable GetDataTable(string cmdText, IEnumerable<Parameter> parameters)
{
    // Create an empty memory table.
    DataTable dataTable = new DataTable();

    // Prepare a connection to the database and command to execute.
    using (SqlConnection connection = new SqlConnection(ConfigurationTool.ConnectionString))
    using (SqlCommand command = new SqlCommand(cmdText, connection))
    {
        command.CommandType = CommandType.StoredProcedure;

        SqlParameterCollection parameterCollection = command.Parameters;
        foreach (Parameter parameter in parameters)
            parameterCollection.Add(CloneParameter(parameter.SqlParameter));

        // Execute the stored procedure and retrieve the results in the table.
        using (SqlDataAdapter dataAdapter = new SqlDataAdapter(command))
        {
             dataAdapter.Fill(dataTable);
        }
    }

    return dataTable;
}

Some things to note here: I was able to get rid of all your try blocks. Not one of them was needed. Also, the SqlDataAdapter.Fill() method will open and close the connection for you, so you don't need that part.

Now let's talk about that CloneParameter() function. I get the impression you feel like it defeats the purpose of your code, which is to try to re-use parameters. I promise you that parameter re-use here is a bad idea. The performance penalty is negligible to the point of non-existence, especially compared to the storedprocedure execution. I left the CloneParameter() implementation to you, for two reason: first of all it's trivial, and second is that we're already outside of my normal data access pattern. What I normally do to add parameters is accept an Action<SqlParameterCollection> delegate, rather than a parameter enumerable. The function is declared more like this:

public IEnumerable<IDataRecord>GetData(string cmdText, Action<SqlParameterCollection> addParameters)

and is called like this:

foreach(var record in GetData("myprocedurename", p => 
  {
      p.Add( /*new parameter here*/ );
      p.Add( /*new parameter here*/ );
    //...
  })
 .Select( /*Returning a IEnumerable rather than a datatable allows me to use it with linq to objects.*/
          /* For example, you could use this spot to convert from DataRecords returned by ADO.Net to business objects */ 
        ))
{
   // use the results here...
}

Since you're filling two tables in a row, it sounds like you have some work to do client side that may justify that vs the DataReader/IEnumerable approach, but I do want to mention this, as most of the time basing your code on a DataReader is the better option.

What I would do in your case with my existing Action-delegate-based pattern and wanting to re-use a duplicate set of parameters as much as possible is have a real, named method that knows how to add the parameters and matches my Action delegate. Then I could just pass that method in, and get the desired parameter re-use.

Answer 2

Interesting question!

It all depends on your Something class. If it was poorly designed and it needs multi-stage cleanup, it forces its idiosyncracies to its clients.

You shouldn't design classes to be like that. If you have interim cleanups to do, encapsulate them in their own classes and use code like this:

using (Something something = new Something()) {
  // ...
  using (SomethingElse somethingElse = something.GiveMeSomethingElse()) {
  }
  // ...
} 

UPDATE:

For your example, it might look like this:

using (SqlConnection connection = new SqlConnection(connectionString)) {
  connection.Open();

  using (SqlCommand command = new SqlCommand("select * from MyTable where id = @id", connection)) {

    // to "reuse" the parameters collection population, just extract this to a separate method      
    command.Parameters.Add(new SqlParameter("id", id));

    // ... execute the command

  }

}

UPDATE 2:

Just do this:

GetDataTable('sp_one', CreateParameters());
GetDataTable('sp_two', CreateParameters());

Answer 3

Dispose should clean up all your unmanaged resources. Having another cleanup method, to perform some functional or database actions for example, is perfectly possible.