Garbage collection, should we rely on it?

Question

In code, say we have:

using (SqlConnection cn = new SqlConnection(ConfigurationManager.ConnectionStrings["LocalSqlServer"].ToString()))
{
    cn.Open();

    // Set all previous settings to inactive
    using (SqlCommand cmd = new SqlCommand("UPDATE tblSiteSettings SET isActive = 0", cn))
    {                            
        cmd.ExecuteNonQuery();
    }

    cn.Close();
}

The cn.close is not technically required as garbage collection will take care of the connection for us.

However, I always like to close it anyway and to not rely on garbage collection. Is this bad? A waste of time? Or considered good practise to not rely on automation?

Thanks for your thoughts and opinions in advance. I'll mark this as community wiki as it's probably subjective.

Answer 1

You should never rely on the GC for this. Raymond Chen's blog article about this is a good starting point. Essentially, if you don't manually Close/Dispose of your connection, then there is no guarantee that it will happen, because otherwise it would only ever happen when Dispose was called from the Finalizer which might not ever happen:

A correctly-written program cannot assume that finalizers will ever run at any point prior to program termination.

Yes, in practice, the finalizer for your connection will probably eventually happen, but even then, you are holding onto a live connection for longer than you actually need to. This can create issues if the database only allows a limited number of live connections at any one time.

What you're doing is considered good practice: when you're done with resources, free them up. If an object is IDisposable, Dispose of it when you can.

Answer 2

First of all - in your example you are using the IDisposable interface, which has nothing to do with GC at all. In essence, your code compiles to this:

SqlConnection cn = null;
try
{
    cn = new SqlConnection(ConfigurationManager.ConnectionStrings["LocalSqlServer"].ToString());
    cn.Open();

    // Set all previous settings to inactive
    using (SqlCommand cmd = new SqlCommand("UPDATE tblSiteSettings SET isActive = 0", cn))
    {
        cmd.ExecuteNonQuery();
    }
    cn.Close();
}
finally
{
    if ( cn != null )
        cn.Dispose();
}

Since cn.Dispose() and cn.Close() are the same in this case - yes, the latter is redundant.

Now, if you wanted to talk about GC, then you'd write like this:

    SqlCOnnection cn = new SqlConnection(ConfigurationManager.ConnectionStrings["LocalSqlServer"].ToString());
    cn.Open();

    // Set all previous settings to inactive
    using (SqlCommand cmd = new SqlCommand("UPDATE tblSiteSettings SET isActive = 0", cn))
    {
        cmd.ExecuteNonQuery();
    }
    cn = null; // Or just leave scope without doing anything else with cn.

In this case it would take the GC to close the opened connection and return it to the pool. And in this case it would mean that you could not predict when that happened. In practice this code would (with high probability) leak SqlConnections and soon you'd run out of them (you'd get a TimeoutException because there would be no available connections in the pool).

So, yes, your above example is the correct way to go. Whenever you use some object that implements IDisposable, wrap it in a using block. And you don't need to bother with the .Close(), although it doesn't hurt either. I personally don't write it though. Less code, less mess.