Do I need to close a connection after grabbing a datareader?

[Alan Silver]

Hello,

I have a generic method in a utility class that grabs an sqldatareader
and returns it. Due to the fact that (AFAIK), you can't close the
database connection before you've read the data, this method doesn't
close it, it just returns the datareader. The calling code uses the
datareader and then just lets it drop out of scope, to be picked up by
the garbage collector.

Is this a problem? A friend of mine suggested to me that not explicitly
closing the connection will leave it open, resulting in the connection
pool being emptied. I've not seen any problems on the server, but that
doesn't prove anything. I previously assumed that the connection would
be closed when the connection object was destroyed. Is this right?

Anyone care to comment? TIA

 

[Mark Rae]

I previously assumed that the connection would be closed when the
connection object was destroyed. Is this right?

Not necessarily...

Anyone care to comment? TIA

You say your method returns a DataReader object to its caller...therefore,
just do the following, and the connection gets closed automatically for you.

return (<MyCommandObject>.ExecuteReader(CommandBehavior.CloseConnection));

 

[Marina Levit [MVP]]

The caller would then be responsible for closing the data reader. If the
datareader was created by calling
ExecuteReader(CommandBehavior.CloseConnection), then closing the data reader
will close the underlying connection. If this is not how the datareader was
created, then closing the data reader will not close the connection and you
will wind up with a connection leak.

The way you are doing it right now will end up in a connection leak under
load, your friend is correct.

The connection will be closed when the object is destroyed - the problem is,
you don't know what that will be. Going out of scope is not the same as
being garbage collected. The GC will run when it needs to reclaim some
memory, not as soon as a variable goes out of scope - but you are likely to
run out of connections in the pool far before then.

 

[Alan Silver]

Thanks to both of you for the replies.

So, am I correct in thinking that if the utility method creates a
command object (cmdCommand), and then does something like...

SqlDataReader dtrData = cmdCommand.ExecuteReader(CommandBehavior.CloseConnection);
return dtrData;

then the calling code just needs to do...

SqlDataReader myData = utilmethod(...);
// do the stuff with the datareader
myData.Close();

....and all will be fine? Please check I've got this right.

Thanks again

The caller would then be responsible for closing the data reader. If the
datareader was created by calling
ExecuteReader(CommandBehavior.CloseConnection), then closing the data reader
will close the underlying connection. If this is not how the datareader was
created, then closing the data reader will not close the connection and you
will wind up with a connection leak.

The way you are doing it right now will end up in a connection leak under
load, your friend is correct.

The connection will be closed when the object is destroyed - the problem is,
you don't know what that will be. Going out of scope is not the same as
being garbage collected. The GC will run when it needs to reclaim some
memory, not as soon as a variable goes out of scope - but you are likely to
run out of connections in the pool far before then.

 

[Marina Levit [MVP]]

Yes, that looks about right.

To avoid any potential problems like this you can just return only
datatables from your helper methods. That way whatever code is using the
result set doesn't have to worry about closing the connection.

Thanks to both of you for the replies.

So, am I correct in thinking that if the utility method creates a command
object (cmdCommand), and then does something like...

SqlDataReader dtrData =
cmdCommand.ExecuteReader(CommandBehavior.CloseConnection);
return dtrData;

then the calling code just needs to do...

SqlDataReader myData = utilmethod(...);
// do the stuff with the datareader
myData.Close();

...and all will be fine? Please check I've got this right.

Thanks again

 

[Mark Rae]

...and all will be fine?

Yep.

 

[Alan Silver]

Yes, that looks about right.
Thanks

To avoid any potential problems like this you can just return only
datatables from your helper methods. That way whatever code is using the
result set doesn't have to worry about closing the connection.

I started using datareaders as I was told (when I first started reading
here) that they were faster and more efficient when you just wanted
forward-only data, say for binding to a repeater. Is this wrong?

Thanks again

 

[Mark Rae]

I started using datareaders as I was told (when I first started reading
here) that they were faster and more efficient when you just wanted
forward-only data, say for binding to a repeater. Is this wrong?

Generally speaking, that's absolutely right.

Of course there will always be someone who can prove that, under a very
particular set of circumstances, they're not as fast as one of the other
ADO.NET objects...

 

[Marina Levit [MVP]]

They might be slightly faster - but unless your application needs every
nanosecond of performance it can get, your users won't notice a difference.

Sometimes code clarity and maintanability outweighs slight performance
benefits. It might be worth it to not use datareaders if it means you won't
have to spend hours looking for the one place in the code where a developer
forgot to close the datareader that is now causing a connection leak.

I'm not saying you shouldn't use datareader, just suggesting that there are
potential coding pitfalls that go along with them.

Yes, that looks about right.
Thanks

To avoid any potential problems like this you can just return only
datatables from your helper methods. That way whatever code is using the
result set doesn't have to worry about closing the connection.

I started using datareaders as I was told (when I first started reading
here) that they were faster and more efficient when you just wanted
forward-only data, say for binding to a repeater. Is this wrong?

Thanks again

 

[Alan Silver]

OK, thanks to both of you (again)

They might be slightly faster - but unless your application needs every
nanosecond of performance it can get, your users won't notice a difference.

Sometimes code clarity and maintanability outweighs slight performance
benefits. It might be worth it to not use datareaders if it means you won't
have to spend hours looking for the one place in the code where a developer
forgot to close the datareader that is now causing a connection leak.

I'm not saying you shouldn't use datareader, just suggesting that there are
potential coding pitfalls that go along with them.

 

[sloan]

My code usually looks like this

IDataReader idr = null;

try
{
idr = SomeMethodToGetAReader();

while (idr.Read())
{
//do stuff
}
finally
{
if (null!=idr)
{
idr.Close();
}
}

If you premature exit the while loop, then use the .Cancel method.

You should write more try/finally blocks, then try/catch or
try/catch/finally blocks. Google "Brad Abrams" and "finally" to find some
info on that.

This is unnecessarily expensive

try{
//try something
}
catch(Exception ex)
{throw ex;}
finally
{
}

the catch/throw isn't doing any good.

 

[Alan Silver]

My code usually looks like this

IDataReader idr = null;

Why do you use IDataReader, instead of (say) SqlDataReader? What's the
benefit?

Thank for the other comments.

try
{
idr = SomeMethodToGetAReader();

while (idr.Read())
{
//do stuff
}
finally
{
if (null!=idr)
{
idr.Close();
}
}

If you premature exit the while loop, then use the .Cancel method.

You should write more try/finally blocks, then try/catch or
try/catch/finally blocks. Google "Brad Abrams" and "finally" to find some
info on that.

This is unnecessarily expensive

try{
//try something
}
catch(Exception ex)
{throw ex;}
finally
{
}

the catch/throw isn't doing any good.

 

[Mark Rae]

Why do you use IDataReader, instead of (say) SqlDataReader? What's the
benefit?

I imagine it's part of a generic DAL which will work with any RDBMS, not
just SQL Server...

I did something similar on a contract last year where the client was
designed to work with a variety of back-end RBDMS e.g. SQL, Sybase, Oracle,
FoxPro, Access, MySQL. Not *too* much of a problem once we'd made the schema
generic enough...

 

[Alan Silver]

I imagine it's part of a generic DAL which will work with any RDBMS, not
just SQL Server...

I did something similar on a contract last year where the client was
designed to work with a variety of back-end RBDMS e.g. SQL, Sybase, Oracle,
FoxPro, Access, MySQL. Not *too* much of a problem once we'd made the schema
generic enough...

OK, thanks for the clarification. As I work exclusively with SQL Server,
I guess I'm safe using SqlDataReader for now.

TA ra

 

[Flinky Wisty Pomm]

FxCop, notably, prefers IDataReader to SqlDataReader if you aren't
using any methods specific to SqlDataReader. If there isn't any
additional cost to using the interface over a concrete class, I'd be
inclined to agree - why make the code less flexible?

 

[Mark Rae]

OK, thanks for the clarification.

Nothing more than an educated guess on my part - no clarification
whatsoever...

 

[Alan Silver]

Nothing more than an educated guess on my part - no clarification
whatsoever...

An educated guess is still clarification to the uneducated ;-)

 

[Alan Silver]

FxCop, notably, prefers IDataReader to SqlDataReader if you aren't
using any methods specific to SqlDataReader. If there isn't any
additional cost to using the interface over a concrete class, I'd be
inclined to agree - why make the code less flexible?

Good point, hadn't thought of it that way. Thanks.