2

I'm helping to debug some code and have the following for loop. Does it open and close a connection to the database on each iteration?

for (int i = 0; i < count; i++)
{
    int num = dataTable.Rows[i]["Location_Id"]
    database.ProcessAlerts("spProcessAlerts", num)
}

And from the database class, here is the relevant portion:

public bool GetConnection()
{
    try
    {
        GeneralLib.GetIniSettings();
        string connectionStrings = GeneralLib.Settings.ConnectionStrings;
        Database.conn = new SqlConnection(connectionStrings);
        Database.conn.Open();
    }
...
public void ProcessAlerts(string ProcedureName, int nByLocationId)
{
    try
{
    if (this.GetConnection())
    {
        SqlCommand sqlCommand = new SqlCommand(ProcedureName, Database.conn);
        sqlCommand.CommandType = CommandType.StoredProcedure;
        SqlParameter sqlParameter = sqlCommand.Parameters.Add("@ByLocation_Id", SqlDbType.Int);
        sqlParameter.Value = nByLocationId;
        sqlCommand.ExecuteNonQuery();
        this.CloseConnection();
    }
}

My intuition leads me to believe the for loop is opening/closing on each iteration but since I don't work with .net at all, I don't really know. I've searched a good bit about this and haven't found a satisfactory answer (like Execution Time Slower with each Iteration of the same SPROC, Keep Sql Connection open for iterating many requests? Or close each step?, Opening and closing database connection for each query and C# SQLConnection pooling). Some posts have said to open the connection when you need it and close it right away while others have said that if you are using connection pooling, the connection isn't really closed, it's just not usable. I don't completely understand this.

Basically this piece of code is supposed to process information so that alerts can be dispatched, and we have recently been experiencing a large time delay in this process. From a log I can see when the for loop starts/stops, and it sometimes takes hours to loop through a couple thousand records. It could also be that spProcessAlerts is taking a lot time to run for certain rows so I am looking into what goes on in there as well.

Community
  • 1
  • 1
Snake_Plissken
  • 390
  • 1
  • 4
  • 14
  • 2
    Have you tried to use SQL profiler to verify? The code reads like it would open a new connection each time – Brian S Jul 28 '14 at 22:36
  • It really looks like it does. You may want to try some kind of singleton pattern http://goo.gl/kmlo implementation for that connection – clarkitect Jul 28 '14 at 22:38
  • 1
    It doesn't take hours to open a few thousand connections. Look elsewhere for the problem. Aren't you suspecting the queries? – usr Jul 28 '14 at 22:43
  • This issue caused by "this.CloseConnection();" on your ProcessAlerts method.. Everytime when calling it closes connection. Than next calling opening new connection. – Ersin Tarhan Jul 28 '14 at 23:05
  • 1
    @GR Envoy +1000. Profile, profile, profile. Course asking on SO is so much easier. – Christopher Painter Jul 28 '14 at 23:38
  • @usr but how should i figure out if opening/closing connections repeatedly is adding significant amounts to the overhead, or you are saying it doesn't and it has to be the sp being called? i am investigating the queries and i am going to also follow gr envoy's suggestion to use the sql server profiler – Snake_Plissken Jul 29 '14 at 16:43
  • Use a profiler or pause the debugger 10 times under load to see where it stops most. Or, add a loop that opens and closes a connection 10 times and see how long it takes. Investigate. – usr Jul 29 '14 at 17:02

3 Answers3

5

Yes... and no.

ADO.Net uses Connection Pooling. So, while you are repeatedly calling Connection.Open() and Connection.Close(), ADO.Net is probably just handing you back the same existing, already open connection.

My preference, though, is still to abstract that kind of thing outside of the loop.

Joel Coehoorn
  • 399,467
  • 113
  • 570
  • 794
1

if you want single connection like pipeline, you can change your code like this :

using(var connection= GetConnectionX()){
connection.Open();
for (int i = 0; i < count; i++)
{
   int num = dataTable.Rows[i]["Location_Id"]
   database.ProcessAlerts("spProcessAlerts", num, connection)
}
}

 public SqlServerConnection GetConnectionX()
 {

    GeneralLib.GetIniSettings();
    string connectionStrings = GeneralLib.Settings.ConnectionStrings;
    return  new SqlConnection(connectionStrings);

 }



  public void ProcessAlerts(string ProcedureName, int nByLocationId , SqlServerConnection connection)
 {
   try
   {

    SqlCommand sqlCommand = new SqlCommand(ProcedureName, connection);
    sqlCommand.CommandType = CommandType.StoredProcedure;
    SqlParameter sqlParameter = sqlCommand.Parameters.Add("@ByLocation_Id", SqlDbType.Int);
    sqlParameter.Value = nByLocationId;
    sqlCommand.ExecuteNonQuery();
}
}
Ersin Tarhan
  • 321
  • 3
  • 11
  • you could also look at using *SqlCommand.Prepare* for a potential performance improvement if your loop has a large number of iterations. See this StackOverflow question for pros and cons: http://stackoverflow.com/questions/2449827/pros-and-cons-of-using-sqlcommand-prepare-in-c and this MSDN article for the docs: http://msdn.microsoft.com/en-us/library/system.data.sqlclient.sqlcommand.prepare(v=vs.110).aspx – Scott Prokopetz Jul 28 '14 at 23:50
0

The way you have your code, yes it looks like you are opening a new connection every time, however you don't have to do it that way.

I'd rework your GetConnection into OpenConnection and CheckConnection, where OpenConnection sets a boolean, and CheckConnection only calls OpenConnection if that boolean is false. Then I would call OpenConnection above your for loop, (and I'd also make a close connection under it)

Aaron
  • 435
  • 3
  • 5
  • What is the connecting closes. It might me better to check the connecting before opening to make sure it's still a valid connection – Brian S Jul 28 '14 at 22:41
  • Rather than an adhoc singleton, i'd suggest perusing [Jon's examples](http://csharpindepth.com/Articles/General/Singleton.aspx) – crthompson Jul 28 '14 at 22:42