Do I need to close and dispose the SQLConnection explicitly?

Question

SqlDataReader rdr = null;
con = new SqlConnection(objUtilityDAL.ConnectionString);
using (SqlCommand cmd = con.CreateCommand())
{
    try
    {
        if (con.State != ConnectionState.Open)
            con.Open();
        cmd.CommandType = CommandType.StoredProcedure;
        cmd.Parameters.Add(Parameter);
        cmd.CommandText = _query;
        rdr = cmd.ExecuteReader();
    }
    catch (Exception ex)
    {
        throw ex;
    }
}

In this above code, sqlconnection is opened inside the managed code. Hence, will the connection object be disposed automatically upon ending the scope of USING?

Your connection object is not inside the scope of a using statement. — Steve, Apr 17 '18 at 06:57

Christos · Answer 1 · 2018-04-17T07:30:16.770

7

You have to care about the disposal of SqlConnection, since it's a disposable object. You can try this:

using(var sqlConnection = new SqlConnection(objUtilityDAL.ConnectionString))
{
    using (SqlCommand cmd = con.CreateCommand())
    {
        // the rest of your code - just replace con with sqlConnection
    }
}

I suggest you replace the con with a local variable - if there isn't already (it does not evident from the code you have posted). There is no need for using a class field for this purpose. Just create a local variable, it would be far more clear to track it.

edited Apr 17 '18 at 07:30

answered Apr 17 '18 at 06:51

Christos

53,228
8
76
108

1

It looks like that `con` might be a field. I don't suggest using a field in the `using` statement. Because once it got disposed in this method and you try to use the field somewhere else you will get an exeption, because the object got disposed. Wouldn't be using a local variable better to avoid above situation? Moreover a field which gets disposed doesn't even have any advantage versus a local variable. Or am I wrong? – L. Guthardt Apr 17 '18 at 07:26
@L.Guthardt By reading the post a bit fast, I did't notice that this might be a class field :). Well said ! As I have written in my update, I don't think that's a good idea to make use of class field for that purpose. – Christos Apr 17 '18 at 07:54
Thank you. :) Very nice that you updated your answer to add this information. +1 – L. Guthardt Apr 17 '18 at 07:59
Thanks so much Christos. Your point suggesting me to use local variable instead of class variable makes sense as it will get disposed inside the method. I would change the logic. Thanks again!! – Chinchan Apr 17 '18 at 08:15

Dmitry Bychenko · Accepted Answer · 2019-11-01T06:40:22.773

You should Dispose every temporary IDisposable instance you create manually, i.e. wrap them into using:

   // Connecton is IDisposable; we create it 
   //   1. manually - new ...
   //   2. for temporary usage (just for the query)
   using (SqlConnection con = new SqlConnection(objUtilityDAL.ConnectionString)) {
     // Check is redundant here: new instance will be closed 
     con.Open();

     // Command is IDisposable
     using (SqlCommand cmd = con.CreateCommand()) {
       cmd.CommandType = CommandType.StoredProcedure;
       cmd.Parameters.Add(Parameter);
       cmd.CommandText = _query;

       // Finally, Reader - yes - is IDisposable 
       using (SqlDataReader rdr = cmd.ExecuteReader()) {
         // while (rdr.Read()) {...}
       }
     }   
   }

Please, notice that

   try {
     ...
   }
   catch (Exception ex) {
     throw ex;
  }

is at least redundant (it does nothing but rethrow the exception, while loosing stack trace) and that's why can be dropped out

Do NOT rethrow exception as it, or you'll lose the stack trace information... Use only `throw;` — Yousha Aleayoub, Nov 01 '19 at 06:29

Zohar Peled · Answer 3 · 2018-04-17T08:46:08.177

In general:

In the .Net framework, nothing gets disposed automatically. Otherwise we wouldn't need the IDisposable interface. The garbage collector can only handle managed resources, so every unmanaged resource must be handled in code in the dispose method.

So as a rule, every instance of every class that is implementing the IDisposable must be disposed either explicitly by calling it's Dispose method or implicitly with the using statement.

As best practice, you should strive to use anything that implements the IDisposable interface as a local variable inside a using statment:

using(var whatever = new SomeIDisposableImplementation())
{
    // use the whatever variable here
}

The using statement is syntactic sugar. the compiler translates it to something like this:

var whatever = new SomeIDisposableImplementation();
try
{
    // use the whatever variable here
}
finally
{
    ((IDisposable)whatever).Dispose();
}

Since the finally block as guaranteed to run regardless of whatever happens in the try block, your IDisposable instance is guaranteed to be properly disposed.

With SqlConnection specifically, in order to return the connection object back to the connection pool, you must dispose it when done (the Dispose method will also close the connection, so you don't need to explicitly close it) - So the correct use of SqlConnection is always as a local variable inside the using statement:

using(var con = new SqlConnection(connectionString))
{
    // do stuff with con here
}

Thanks so much @Zohar Peled – Chinchan Apr 17 '18 at 13:25 — Chinchan, Apr 17 '18 at 13:25

Do I need to close and dispose the SQLConnection explicitly?

3 Answers3

Linked