103

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?

Jon Ownbey
  • 1,428
  • 2
  • 12
  • 18

4 Answers4

145

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
  • 5,565
  • 4
  • 28
  • 21
  • 30
    You don't need the .Close() statement in either sample: it's handled by the .Dispose() call. – Joel Coehoorn Apr 13 '09 at 14:34
  • 1
    This sample doesn't show the underlying connection being closed/disposed, which is the most important thing. – Joe Apr 13 '09 at 15:46
  • 1
    I don't get it when would the reader be null after "cmd.ExecuteReader"? – Andrei Rînea Apr 14 '09 at 00:10
  • 7
    Probably want to check if it .HasRows rather then null. – JonH Nov 23 '09 at 13:13
  • I've just run into an error that is thrown when I try to call ExecuteReader on a stored procedure that does not return any results. When this happens, ExecuteReader fails, throws an exception, and returns null:) – Andrew Mar 22 '12 at 14:17
  • Also, I agree that the using statements are great, if you're not looking for specific exceptions that are being thrown. If, on the other hand, you are, then I prefer to use try-catch clauses, although I admit that the majority of my code sticks to the using clauses. – Andrew Mar 22 '12 at 14:19
  • 4
    @Andrew If ExecuteReader throws an exception, how can it return null? – csauve Jun 18 '12 at 21:31
  • 7
    @JohH: the while (reader.Read()) in the example accomplishes the same as .HasRows, and you need to .Read anyways to advance the reader forward to the first row. – csauve Jun 18 '12 at 21:33
  • 1
    @csauve You're right, I guess it must not have returned null. I'm not sure why I was looking at the value of the SqlDataReader variable. – Andrew Jun 19 '12 at 01:13
  • actually you don't even need brackets to use `using` – serge Jan 04 '21 at 16:41
66

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 = new SqlConnection(...);
    try
    {
        connection.Open();
        using(SqlCommand command = new SqlCommand(commandText, connection))
        {
            return command.ExecuteReader(CommandBehavior.CloseConnection);
        }
    }
    catch
    {
        // Close connection before rethrowing
        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
  • 122,218
  • 32
  • 205
  • 338
  • 1
    In the second code snippet where the method returns a SqlDataReader the command is not disposed. Is that ok and is it ok to dispose the command (enclose it in a using block) and then return the reader? – alwayslearning Oct 21 '11 at 06:36
  • 1
    @alwayslearning that's exactly the scenario that I have......can you close/dispose of the SqlCommand when you are returning the SqlDataReader to the caller? – ganders May 18 '12 at 20:15
  • 2
    This is bad. If you *REALLY* can't bear to use `using`s then call dispose in the `finally {}` block after catch. The way this is written, successful commands would never be closed or disposed. – smdrager Oct 02 '12 at 20:27
  • 5
    @smdrager, if you read the answer closer, he is talking about a method that returns a reader. If you use .ExecuteReader(CommandBehavior.CloseConnection); then by disposing the READER, the connection will be closed. So the calling method only needs to wrap the resulting reader in a using statement. using(var rdr = SqlHelper.GetReader()){ //... } if you were close it in the finally block, then your reader would fail to read because the connection is closed. – Sinaesthetic Dec 30 '12 at 00:04
  • 1
    @ganders - coming back to this old post: yes you can and probably should dispose the SqlCommand - updated the example to do so. – Joe May 24 '13 at 10:32
  • 1
    This is a much more detailed and usable answer as it points out multiple use cases. – AlexVPerl Nov 22 '15 at 01:24
  • 1
    Disposing the command disposes the reader and any attempt to read fails, the command should not be disposed in this case. – jjxtra Jun 10 '19 at 01:10
15

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

Kon
  • 27,113
  • 11
  • 60
  • 86
  • 1
    Fair enough. However, does it actually make a difference in performance if there is no using statement? – Jon Ownbey Apr 13 '09 at 14:30
  • 2
    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 Apr 13 '09 at 14:34
  • 2
    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 Apr 13 '09 at 14:35
  • 7
    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 Apr 13 '09 at 14:35
6

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

J.W.
  • 17,991
  • 7
  • 43
  • 76