DB connections not closed fast enough?

Brent · Nov 18, 2005

I'm quickly running out of available db connections with the code below.
I like to think I'm closing everything as I go along, but I watch the
connections shoot up as soon as the the method getIDs comes into play.
Does anyone see anything obvious? It's almost as if my code can't close
a DB connection as fast as it can open it...

Thanks for any help!

--Brent

================================
Code Fragment
================================
//after creating array string[] ids

for(int i = 0;i < 50;i++)
{
string theseids = getIDs(Int32.Parse(ids));

if (theseids != "0")
{
Response.Write(cusips.ToString());
}

}

public string getIDs(int id)
{
StringBuilder ids = new StringBuilder();

//Set up sql connection & sql statement

myConn.Open(); //open connection

//set up datareader and loop thru rows,
//appending to the StringBuilder at each
//loop

if (ids.ToString().Length > 0)
{
return ids.ToString();
}
else
{
return "0";
}

if(myConn != null){myConn.Dispose();}

}

Andrew Robinson · Nov 18, 2005

Kind of an incomplete code snippet, but how about closing your connection
before you dispose it?

Brent · Nov 18, 2005

Hmmm...tried that, with no luck...

The full code, which isn't much more, is here:

===================================================
public void writeIDs()
{

StringBuilder sbsql = new StringBuilder();

//get array of ids we want to look at.
string[] filingIDs = getFilingIDs();

for(int i = 0;i < 50;i++)
{
string theseids = getIDs(Int32.Parse(filingIDs));
if (theseids!= "0")
{
output(theseids.ToString());
}

}

}
public string getIDs(int filingid)
{
StringBuilder idlist = new StringBuilder();
string sql = "select thisid from holdings where thisid = "+ filingid;

MySqlConnection myConn;
myConn = new
MySqlConnection(ConfigurationSettings.AppSettings["connString"]);

MySqlCommand myCommand = new MySqlCommand(sql, myConn);

myConn.Open();

MySqlDataReader dr = myCommand.ExecuteReader();

if (dr.HasRows)
{
while (dr.Read())
{
if (dr["thisid"].ToString().Length > 0)
{
idlist.Append("'" + dr["thisid"].ToString()+"',");
}
}
}
int thislength= idlist.ToString().Length;
if (thislength> 0)
{
return idlist.ToString().Substring(0,thislength);
}
else
{
return "0";
}
if(myConn != null){myConn.Close();myConn.Dispose();}

}

public string[] getFilingIDs()
{
StringBuilder IDs = new StringBuilder();
string sql = "select filingid from filings where filing_type = 1 and
parseid > 0 limit 100 ";
MySqlConnection myConn = new
MySqlConnection(ConfigurationSettings.AppSettings["connString"]);
MySqlCommand myCommand = new MySqlCommand(sql, myConn);

myConn.Open();

MySqlDataReader dr = myCommand.ExecuteReader();

if (dr.HasRows)
{
while (dr.Read())
{
IDs.Append(dr["filingid"].ToString()+",");

}
}
int filingIDLength = filingIDs.ToString().Length-1;
return filingIDs.ToString().Substring(0,filingIDLength).Split(',');
if(myConn != null){myConn.Close();myConn.Dispose();}
}
===========================

Peter Rilling · Nov 19, 2005

Maybe the connections are pooling.

Guest · Nov 19, 2005

you have a loop of 50 connections being opened and closed.
This:
string sql = "select thisid from holdings where thisid = "+ filingid;

could be converted to something like:

string sql = "select thisid from holdings where thisid in (id1, id2, ....)";
and the whole thing done on a single SQL Call.

In addition, I'd REALLY discourage the use of non-parameterized text type
queries instead of stored procedures. If you want to take the RPC path
through SQL Server instead of the language path, you need to set
CommandType.StoredProcedure, use a stored proc, and pass it a SqlParameter
Collection.

Check here for some details on why this is so troublesome (courtesy of Gert
Drapers of MS):

http://petesbloggerama.blogspot.com/2004/11/adonet-and-sql-server-optimization.html

--Peter

--
Co-founder, Eggheadcafe.com developer portal:
http://www.eggheadcafe.com
UnBlog:
http://petesbloggerama.blogspot.com

Brent" <""b b i g l e r "@ y a h o o . said:
Hmmm...tried that, with no luck...

The full code, which isn't much more, is here:

===================================================
public void writeIDs()
{

StringBuilder sbsql = new StringBuilder();

//get array of ids we want to look at.
string[] filingIDs = getFilingIDs();

for(int i = 0;i < 50;i++)
{
string theseids = getIDs(Int32.Parse(filingIDs));
if (theseids!= "0")
{
output(theseids.ToString());
}

}

}
public string getIDs(int filingid)
{
StringBuilder idlist = new StringBuilder();
string sql = "select thisid from holdings where thisid = "+ filingid;

MySqlConnection myConn;
myConn = new
MySqlConnection(ConfigurationSettings.AppSettings["connString"]);

MySqlCommand myCommand = new MySqlCommand(sql, myConn);

myConn.Open();

MySqlDataReader dr = myCommand.ExecuteReader();

if (dr.HasRows)
{
while (dr.Read())
{
if (dr["thisid"].ToString().Length > 0)
{
idlist.Append("'" + dr["thisid"].ToString()+"',");
}
}
}
int thislength= idlist.ToString().Length;
if (thislength> 0)
{
return idlist.ToString().Substring(0,thislength);
}
else
{
return "0";
}
if(myConn != null){myConn.Close();myConn.Dispose();}

}

public string[] getFilingIDs()
{
StringBuilder IDs = new StringBuilder();
string sql = "select filingid from filings where filing_type = 1 and
parseid > 0 limit 100 ";
MySqlConnection myConn = new
MySqlConnection(ConfigurationSettings.AppSettings["connString"]);
MySqlCommand myCommand = new MySqlCommand(sql, myConn);

myConn.Open();

MySqlDataReader dr = myCommand.ExecuteReader();

if (dr.HasRows)
{
while (dr.Read())
{
IDs.Append(dr["filingid"].ToString()+",");

}
}
int filingIDLength = filingIDs.ToString().Length-1;
return filingIDs.ToString().Substring(0,filingIDLength).Split(',');
if(myConn != null){myConn.Close();myConn.Dispose();}
}
===========================

Andrew said:

Kind of an incomplete code snippet, but how about closing your connection
before you dispose it?

Click to expand...

Bjorn Abelli · Nov 19, 2005

...

The full code, which isn't much more, is here:

To begin with, you've got the closing unreachable, as they're *after* the
return statements...

public string getIDs(int filingid)
{ [snip]
if (thislength> 0)
{
return idlist.ToString().Substring(0,thislength);
}
else
{
return "0";
}

// Is this really compilable?

if(myConn != null){myConn.Close();myConn.Dispose();}
}

public string[] getFilingIDs()
{

[snip]

// Is this really compilable?

return filingIDs.ToString().Substring(0,filingIDLength).Split(',');
if(myConn != null){myConn.Close();myConn.Dispose();}
}

Secondly, why so many separate connections?

You can open a single connection in e.g. "writeIDs", and send it as a
parameter to getIDs and getFilingIDs, and then finally close it at the end
of "writeIDs", just as an example.

That is, if you don't want it on an even higher level...

// Bjorn A

carion1 · Nov 19, 2005

Maintain a single connection...opening connections is expensive. Just
because you call dispose doesn't mean the GC cleans it up right away. Use
procs and parameterized commands like the others mentioned. 99.9% of the
communication between my apps and the db is handled through sp's which
allows me to restrict the db users rights since they don't need any rights
on any other objects other than sp's. If that user is then compromised its
fairly useless in that all they can do is execute the queries I wrote. Try
not to make the same mistake with dynamic sql it seems a lot of programmers
make in stored procedures. Business logic has no place in the db layer much
less the db.

If nothing else at the very least escape " ' " when coding inline sql to
HELP prevent sql injection attacks.

--

Derek Davis
(e-mail address removed)

Brent said:
Hmmm...tried that, with no luck...

The full code, which isn't much more, is here:

===================================================
public void writeIDs()
{

StringBuilder sbsql = new StringBuilder();

//get array of ids we want to look at.
string[] filingIDs = getFilingIDs();

for(int i = 0;i < 50;i++)
{
string theseids = getIDs(Int32.Parse(filingIDs));
if (theseids!= "0")
{
output(theseids.ToString());
}

}

} public string getIDs(int filingid)
{
StringBuilder idlist = new StringBuilder();
string sql = "select thisid from holdings where thisid = "+ filingid;

MySqlConnection myConn;
myConn = new
MySqlConnection(ConfigurationSettings.AppSettings["connString"]);

MySqlCommand myCommand = new MySqlCommand(sql, myConn);

myConn.Open();

MySqlDataReader dr = myCommand.ExecuteReader();
if (dr.HasRows)
{
while (dr.Read())
{
if (dr["thisid"].ToString().Length > 0)
{
idlist.Append("'" + dr["thisid"].ToString()+"',");
}
}
}
int thislength= idlist.ToString().Length;
if (thislength> 0)
{
return idlist.ToString().Substring(0,thislength);
}
else
{
return "0";
}
if(myConn != null){myConn.Close();myConn.Dispose();}

}

public string[] getFilingIDs()
{
StringBuilder IDs = new StringBuilder();
string sql = "select filingid from filings where filing_type = 1 and
parseid > 0 limit 100 ";
MySqlConnection myConn = new
MySqlConnection(ConfigurationSettings.AppSettings["connString"]);
MySqlCommand myCommand = new MySqlCommand(sql, myConn);

myConn.Open();

MySqlDataReader dr = myCommand.ExecuteReader();
if (dr.HasRows)
{
while (dr.Read())
{
IDs.Append(dr["filingid"].ToString()+",");

}
}
int filingIDLength = filingIDs.ToString().Length-1;
return filingIDs.ToString().Substring(0,filingIDLength).Split(',');
if(myConn != null){myConn.Close();myConn.Dispose();}
}
===========================

Andrew said:

Kind of an incomplete code snippet, but how about closing your connection
before you dispose it?

Click to expand...

Kevin Spencer · Nov 19, 2005

You're not closing your DataReader. In addition, you are recreating the same
Connection 50 times inside a loop. While Connection Pooling will re-use the
Connection, this is still not a good idea memory-wise, temporarily. The
Connection class is still re-created each time through the loop. Since you
are re-using the Connection inside the same Method scope, in fact in a loop,
it would be better to create the Connection 1 time before the loop, and
re-use it with a new DataReader each time (which must be closed), and close
the Connection immediately after the loop. Be sure to either use a "using"
statement to ensure that the Connection is disposed, or a try/catch block,
in case anything untoward happens.

--
HTH,

Kevin Spencer
Microsoft MVP
..Net Developer
If you push something hard enough,
it will fall over.
- Fudd's First Law of Opposition

Brent said:
Hmmm...tried that, with no luck...

The full code, which isn't much more, is here:

===================================================
public void writeIDs()
{

StringBuilder sbsql = new StringBuilder();

//get array of ids we want to look at.
string[] filingIDs = getFilingIDs();

for(int i = 0;i < 50;i++)
{
string theseids = getIDs(Int32.Parse(filingIDs));
if (theseids!= "0")
{
output(theseids.ToString());
}

}

} public string getIDs(int filingid)
{
StringBuilder idlist = new StringBuilder();
string sql = "select thisid from holdings where thisid = "+ filingid;

MySqlConnection myConn;
myConn = new
MySqlConnection(ConfigurationSettings.AppSettings["connString"]);

MySqlCommand myCommand = new MySqlCommand(sql, myConn);

myConn.Open();

MySqlDataReader dr = myCommand.ExecuteReader();
if (dr.HasRows)
{
while (dr.Read())
{
if (dr["thisid"].ToString().Length > 0)
{
idlist.Append("'" + dr["thisid"].ToString()+"',");
}
}
}
int thislength= idlist.ToString().Length;
if (thislength> 0)
{
return idlist.ToString().Substring(0,thislength);
}
else
{
return "0";
}
if(myConn != null){myConn.Close();myConn.Dispose();}

}

public string[] getFilingIDs()
{
StringBuilder IDs = new StringBuilder();
string sql = "select filingid from filings where filing_type = 1 and
parseid > 0 limit 100 ";
MySqlConnection myConn = new
MySqlConnection(ConfigurationSettings.AppSettings["connString"]);
MySqlCommand myCommand = new MySqlCommand(sql, myConn);

myConn.Open();

MySqlDataReader dr = myCommand.ExecuteReader();
if (dr.HasRows)
{
while (dr.Read())
{
IDs.Append(dr["filingid"].ToString()+",");

}
}
int filingIDLength = filingIDs.ToString().Length-1;
return filingIDs.ToString().Substring(0,filingIDLength).Split(',');
if(myConn != null){myConn.Close();myConn.Dispose();}
}
===========================

Andrew said:

Kind of an incomplete code snippet, but how about closing your connection
before you dispose it?

Click to expand...

John Chrisman · Nov 20, 2005

Yes, I see the problem. You are not disposing of the connections because you
are returning from the methods before calling dispose on the connection
object. Remember, return immediately exits the method. Also, good
programming practice is to never have more than one return statement in your
method and it should always be the last statement in the method.

I posted the corrected code below.

Also, everyone else is right about opening and closing so many connections
so quickly. You should open the connection in the writeIDs method and pass
it as an argument to the other two methods. It will work the way I corrected
it below, but it would be much more efficient to open the connection once,
do ALL of your processing, then close the connection.

public void writeIDs()
{
StringBuilder sbsql = new StringBuilder();

//get array of ids we want to look at.
string[] filingIDs = getFilingIDs();

for(int i = 0;i < 50;i++)
{
string theseids = getIDs(Int32.Parse(filingIDs));
if (theseids!= "0")
{
output(theseids.ToString());
}
}
}

public string getIDs(int filingid)
{
StringBuilder idlist = new StringBuilder();
string sql = "select thisid from holdings where thisid = "+ filingid;
string returnValue = null;

MySqlConnection myConn;
myConn = new
MySqlConnection(ConfigurationSettings.AppSettings["connString"]);

MySqlCommand myCommand = new MySqlCommand(sql, myConn);

myConn.Open();

MySqlDataReader dr = myCommand.ExecuteReader();

if (dr.HasRows)
{
while (dr.Read())
{
if (dr["thisid"].ToString().Length > 0)
{
idlist.Append("'" + dr["thisid"].ToString()+"',");
}
}
}
int thislength= idlist.ToString().Length;
if (thislength> 0)
{
returnValue = idlist.ToString().Substring(0,thislength);
}
else
{
returnValue = "0";
}
if(myConn != null)
{
myConn.Close();
myConn.Dispose();
}
return returnValue;
}

public string[] getFilingIDs()
{
StringBuilder IDs = new StringBuilder();
string sql = "select filingid from filings where filing_type = 1 and
parseid > 0 limit 100 ";
MySqlConnection myConn = new
MySqlConnection(ConfigurationSettings.AppSettings["connString"]);
MySqlCommand myCommand = new MySqlCommand(sql, myConn);

myConn.Open();

MySqlDataReader dr = myCommand.ExecuteReader();

if (dr.HasRows)
{
while (dr.Read())
{
IDs.Append(dr["filingid"].ToString()+",");
}
}
int filingIDLength = filingIDs.ToString().Length-1;
if(myConn != null)
{
myConn.Close();
myConn.Dispose();
}
return filingIDs.ToString().Substring(0,filingIDLength).Split(',');
}

Brent said:

Hmmm...tried that, with no luck...

The full code, which isn't much more, is here:

===================================================
public void writeIDs()
{

StringBuilder sbsql = new StringBuilder();

//get array of ids we want to look at.
string[] filingIDs = getFilingIDs();

for(int i = 0;i < 50;i++)
{
string theseids = getIDs(Int32.Parse(filingIDs));
if (theseids!= "0")
{
output(theseids.ToString());
}

}

}
public string getIDs(int filingid)
{
StringBuilder idlist = new StringBuilder();
string sql = "select thisid from holdings where thisid = "+ filingid;

MySqlConnection myConn;
myConn = new
MySqlConnection(ConfigurationSettings.AppSettings["connString"]);

MySqlCommand myCommand = new MySqlCommand(sql, myConn);

myConn.Open();

MySqlDataReader dr = myCommand.ExecuteReader();

if (dr.HasRows)
{
while (dr.Read())
{
if (dr["thisid"].ToString().Length > 0)
{
idlist.Append("'" + dr["thisid"].ToString()+"',");
}
}
}
int thislength= idlist.ToString().Length;
if (thislength> 0)
{
return idlist.ToString().Substring(0,thislength);
}
else
{
return "0";
}
if(myConn != null){myConn.Close();myConn.Dispose();}

}

public string[] getFilingIDs()
{
StringBuilder IDs = new StringBuilder();
string sql = "select filingid from filings where filing_type = 1 and
parseid > 0 limit 100 ";
MySqlConnection myConn = new
MySqlConnection(ConfigurationSettings.AppSettings["connString"]);
MySqlCommand myCommand = new MySqlCommand(sql, myConn);

myConn.Open();

MySqlDataReader dr = myCommand.ExecuteReader();

if (dr.HasRows)
{
while (dr.Read())
{
IDs.Append(dr["filingid"].ToString()+",");

}
}
int filingIDLength = filingIDs.ToString().Length-1;
return filingIDs.ToString().Substring(0,filingIDLength).Split(',');
if(myConn != null){myConn.Close();myConn.Dispose();}
}
===========================

Andrew said:

Kind of an incomplete code snippet, but how about closing your connection
before you dispose it?

Click to expand...

Click to expand...

Marcus Andrén · Nov 20, 2005

Yes, I see the problem. You are not disposing of the connections because you
are returning from the methods before calling dispose on the connection
object. Remember, return immediately exits the method. Also, good
programming practice is to never have more than one return statement in your
method and it should always be the last statement in the method.

I have to disagree strongly with this. Having a single return
statement as the last statement in the method is not in any way better
programming practice. All it does is introduce an extra variable whose
single purpose is to hold a return value.

While your solution works partially, it still suffers from the same
flaw as the original program. If the method throws an exception, the
database connection will remain open and the next time the method is
called it will fail.

The real solution is to use either try-finally or the using statement.

Marcus Andrén · Nov 20, 2005

You should use the using statment (or try-finally) to make sure that
the sqlconnection is disposed of.

// using will automatically dispose the connection when leaving the
// braces
using(SqlConnection conn = new SqlConnection(connectionString))
{
// Do stuff here
}

This is syntactic sugar for the following code which is the generic
version.

SqlConnection conn = new SqlConnection(connectionString)
try
{
//Do stuff here
}
finally
{
if(conn != null)
conn.Dispose();
}

DB connections not closed fast enough?

Brent

Andrew Robinson

Brent

Peter Rilling

Guest

Bjorn Abelli

carion1

Kevin Spencer

John Chrisman

Marcus Andrén

Marcus Andrén