Returning the SqlDataReader

Question

I am trying to either pass a reader by reference or to have one returned. And I am having issues with it on the return.

public static SqlDataReader GetSql(string businessUnit, string taskId)
{
            const string connstring = "Connection string";
            SqlConnection conn = new SqlConnection(connstring);
            SqlCommand command = new SqlCommand();
            SqlDataReader reader;
            try
            {
                conn.Open();
                command.Connection = conn;
                command.CommandType = CommandType.Text;
                command.CommandText =
                    "SELECT * FROM Audits WHERE BusinessUnit = @BU AND TaskID = @TID";
                command.Parameters.AddWithValue("@BU", businessUnit);
                command.Parameters.AddWithValue("@TID", taskId);
                return reader = command.ExecuteReader(CommandBehavior.CloseConnection);
            }
            catch (Exception ex)
            {
                return null;
            }
            finally
            {
                conn.Close();
            }
}


SqlDataReader reader = QaqcsqlLib.GetSql("job", "Task1");

if (reader.HasRows)
{
   while (reader.Read())
      MessageBox.Show(reader[0].ToString());
}

but I get the following error

Invalid attempt to call HasRows when reader is closed.

Any Ideas?

Jon Skeet · Accepted Answer · 2013-05-09 19:26:31Z

12

This is the problem:

finally
{
    conn.Close();
}

You're closing the connection before the method returns. The reader isn't going to be able to function without an open connection.

(It's not clear why you've got the reader variable at all, given that you only use it when returning.)

Returning a SqlDataReader in a method which itself opens the connection is generally tricky - because it means you don't have a nice way of closing the connection. Better would be to let the caller pass in the connection, at which point you can have:

using (var connection = new SqlConnection(...))
{
    using (var reader = QaqcsqlLib.GetSql(connection, "job", "Task1"))
    {
        // Use the reader here
    }
}

EDIT: As noted by Scott, your use of CommandBehavior.CloseConnection would allow closing the reader to close the connection. However, it makes other things trickier. For example, you'd have to dispose of the connection if an exception occurs (because then the caller won't have a chance), but not otherwise - I still prefer my approach.

edited May 9, 2013 at 19:26

answered May 9, 2013 at 18:18

Jon Skeet

1.5m893 gold badges9.3k silver badges9.3k bronze badges

Sign up to request clarification or add additional context in comments.

5 Comments

Herrozerro Over a year ago

so, I should pass both a connection and a reader? I think i get it now. I was thinking that a reader could be executed then read disconnected. would a dataadapter work like I describe?

Seth Moore Over a year ago

@Herrozerro: The reader doesn't actually contain all of the data from your query, it fetches it as you need it to conserve memory. msdn.microsoft.com/en-us/library/haa3afyz(v=vs.80).aspx

Jon Skeet Over a year ago

@Herrozerro: No, you pass a connection, and return a reader associated with that connection. A DataAdapter wouldn't do it either - but you could return a DataTable if you really wanted.

Scott Chamberlain Over a year ago

@JonSkeet I have to disagree with you. He is passing in CommandBehavior.CloseConnection to ExecuteReader, when he disposes of his reader it will close the underlying connection for him, the only correction he needed to make is remove the finally and add a using block around his reader.

Jon Skeet Over a year ago

@ScottChamberlain: That's true, good point... (And I'll edit something along those lines into the ansewr) although I would personally still prefer the approach taken here, as it's clearer IMO.

Community · Accepted Answer · 2017-05-23 12:29:22Z

John is right about why you are having the problem but you don't need to pass in a connection.

I can see that you are already using the SqlCommand.ExecuteReader(CommandBehavior) overload when you start your reader and passing in CommandBehavior.CloseConnection. However the thing you are doing wrong is you are never disposing of the reader that is returned from your function. Remove the finally block then wrap your returned reader in a using block. This will close the underlying connection for you when you exit the block (becuse you passed in CommandBehavior.CloseConnection)

using(SqlDataReader reader = QaqcsqlLib.GetSql("job", "Task1"))
{
    while (reader != null && reader.Read()) //Added null check as your GetSql could return null.
      MessageBox.Show(reader[0].ToString());
}

I also stripped out reader.HasRows because it is unnessasary. If no results where returned the first call to reader.Read() will return false and the code inside the while loop will never execute.

You still need to close the connection when a exception occurs but you can just move the close from the finally in to the catch.

catch (Exception ex)
{
    conn.Dispose(); //Disposing closes the connection.
    return null;
}

Rolando · Accepted Answer · 2013-05-09 18:20:20Z

0

you can try follow the next example http://msdn.microsoft.com/en-us/library/haa3afyz.aspx .

You shouldn't call to close method before to use your reader.

answered May 9, 2013 at 18:20

Rolando

7621 gold badge15 silver badges42 bronze badges

Comments

Community · Accepted Answer · 2017-05-23 11:49:16Z

0

Jon Skeet and Scott Chamberlain are both correct. If you wish to keep with your way of structuring the code (as opposed to Jon Skeet's approach), you will need to modify your code so that the connection is closed should an error occur while executing the SqlCommand.

public static SqlDataReader GetSql(string businessUnit, string taskId)
{
    const string connstring = "Connection string";
    SqlConnection conn = null;
    SqlCommand command = null;
    SqlDataReader reader = null;
    try
    {
        conn = new SqlConnection(connstring);
        command = new SqlCommand();
        command.Connection = conn;
        command.CommandType = CommandType.Text;
        command.CommandText = "SELECT * FROM Audits WHERE BusinessUnit = @BU AND TaskID = @TID";
        command.Parameters.AddWithValue("@BU", businessUnit);
        command.Parameters.AddWithValue("@TID", taskId);
        conn.Open();
        reader = command.ExecuteReader(CommandBehavior.CloseConnection);
        conn = null;
        return reader;
    }
    catch (Exception ex)
    {
        return null;
    }
    finally
    {
        if (conn != null) conn.Dispose();
        if (command != null) command.Dispose();
    }
}

edited May 23, 2017 at 11:49

CommunityBot

11 silver badge

answered May 9, 2013 at 21:29

Ɖiamond ǤeezeƦ

3,3413 gold badges32 silver badges42 bronze badges

2 Comments

Scott Chamberlain Over a year ago

That conn = null + handling it in the finally block is a really weird way of doing it. I think if you planned on explicitly disposing in the event of an exception, do it in the catch block.

Ɖiamond ǤeezeƦ Over a year ago

@ScottChamberlain That's the way it is suggested to do it on MSDN msdn.microsoft.com/query/… - CA2000: Dispose objects before losing scope. If you do it via the catch block, FxCop warns that SqlConnection is not disposed of along all exception paths.

Collectives™ on Stack Overflow

Returning the SqlDataReader

4 Answers 4

5 Comments

Comments

Comments

2 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

5 Comments

Comments

Comments

2 Comments

Your Answer

Sign up or log in

Post as a guest

Related