views:

1889

answers:

5

I'm working with legacy code here and there are many instances of SQLDataReader that are never closed or disposed. The connection is closed but, I am not sure if it is necessary to manage the reader manually.

Could this cause a slowdown in performance?

+4  A: 

To be safe, wrap every SqlDataReader object in a using statement.

Kon
Fair enough. However, does it actually make a difference in performance if there is no using statement?
Jon Ownbey
A using statement is the same as wrapping the DataReader code in a try..finally... block, with the close/dispose method in the finally section. Basically, it just "guarantees" that the object will be disposed of properly.
Todd Benning
This is straight from the link I provided: "The using statement ensures that Dispose is called even if an exception occurs while you are calling methods on the object."
Kon
Continued... "You can achieve the same result by putting the object inside a try block and then calling Dispose in a finally block; in fact, this is how the using statement is translated by the compiler."
Kon
+3  A: 

Just wrap your SQLDataReader with "using" statement. That should take care of most of your issues.

J.W.
+9  A: 

Try to avoid using readers like this:

SqlConnection connection = new SqlConnection("connection string");
SqlCommand cmd = new SqlCommand("SELECT * FROM SomeTable", connection);
SqlDataReader reader = cmd.ExecuteReader();
connection.Open();
if (reader != null)
{
      while (reader.Read())
      {
              //do something
      }
}
reader.Close(); // <- too easy to forget
reader.Dispose(); // <- too easy to forget
connection.Close(); // <- too easy to forget

Instead, wrap them in using statements:

using(SqlConnection connection = new SqlConnection("connection string"))
{

    connection.Open();

    using(SqlCommand cmd = new SqlCommand("SELECT * FROM SomeTable", connection))
    {
     using (SqlDataReader reader = cmd.ExecuteReader())
     {
      if (reader != null)
      {
       while (reader.Read())
       {
           //do something
       }
      }
     } // reader closed and disposed up here

    } // command disposed here

} //connection closed and disposed here

The using statement will ensure correct disposal of the object and freeing of resources.

If you forget then you are leaving the cleaning up to the garbage collector, which could take a while.

Codebrain
You don't need the .Close() statement in either sample: it's handled by the .Dispose() call.
Joel Coehoorn
Fine, I marked the close as optional in the code
Codebrain
This sample doesn't show the underlying connection being closed/disposed, which is the most important thing.
Joe
I don't get it when would the reader be null after "cmd.ExecuteReader"?
Andrei Rinea
Probably want to check if it .HasRows rather then null.
JonH
+2  A: 

Note that disposing a SqlDataReader instantiated using SqlCommand.ExecuteReader() will not close/dispose the underlying connection.

There are two common patterns. In the first, the reader is opened and closed within the scope of the connection:

using(SqlConnection connection = ...)
{
    connection.Open();
    ...
    using(SqlCommand command = ...)
    {
        using(SqlDataReader reader = command.ExecuteReader())
        {
            ... do your stuff ...
        } // reader is closed/disposed here
    } // command is closed/disposed here
} // connection is closed/disposed here

Sometimes it's convenient to have a data access method open a connection and return a reader. In this case it's important that the returned reader is opened using CommandBehavior.CloseConnection, so that closing/disposing the reader will close the underlying connection. The pattern looks something like this:

public SqlDataReader ExecuteReader(string commandText)
{
    SqlConnection connection = null;
    SqlCommand command = null;
    try
    {
        connection = new SqlConnection(...);
        connection.Open();
        command = new SqlCommand(commandText, connection);
        return command.ExecuteReader(CommandBehavior.CloseConnection);
    }
    catch
    {
        // Close connection before rethrowing
        command.Close();
        connection.Close();
        throw;
    }
}

and the calling code just needs to dispose the reader thus:

using(SqlDataReader reader = ExecuteReader(...))
{
    ... do your stuff ...
} // reader and connection are closed here.
Joe
There is not a Close() method on the SqlCommand object.
Dave
A: 

Joe, There is no SqlCommand.Close() method

alexndr