Is My Data Access Leaking Connections?

Question

Folks

I have an N-Tiered ASP.Net Web Forms application which uses Enterprise Library 5.0 for data persistence. Recently I have noticed my application has been spitting out the following error

Timeout expired. The timeout period elapsed prior to obtaining a connection from the pool

This would point to leaking connections or connections not being closed. Please can someone look at my Data Access class and let me know if they see any issues where connections are not being closed properly

Public Class DataAccess

    Private db As Database = DatabaseFactory.CreateDatabase()

    Public Function ExecuteNonQuery(ByVal params() As SqlParameter, ByVal strSproc As String) As Integer

        Dim intReturnValue As Integer = 0
        Dim i As Integer

        Using cmd As DbCommand = db.GetStoredProcCommand(strSproc)

            cmd.CommandTimeout = 120

            For i = 0 To params.Length - 1
                db.AddInParameter(cmd, params(i).ParameterName.ToString, params(i).DbType, params(i).Value)
            Next

            db.AddParameter(cmd, "return_value", DbType.Int32, ParameterDirection.ReturnValue, "", DataRowVersion.Default, 0)

            db.ExecuteNonQuery(cmd)
            intReturnValue = Int32.Parse(db.GetParameterValue(cmd, "@return_value"))

        End Using

        Return intReturnValue

    End Function

    Public Function ExecuteDataReader(ByVal params() As SqlParameter, ByVal SProc As String) As SqlDataReader

        Dim i As Integer

        Dim dr As SqlDataReader

        Using cmd As DbCommand = db.GetStoredProcCommand(SProc)

            cmd.CommandTimeout = 120

            For i = 0 To params.Length - 1
                db.AddInParameter(cmd, params(i).ParameterName.ToString, params(i).DbType, params(i).Value)
            Next

            dr = TryCast(DirectCast(db.ExecuteReader(cmd), RefCountingDataReader).InnerReader, SqlDataReader)

        End Using

        Return dr

    End Function

    Public Function ExecuteDataSet(ByVal Sproc As String, ByVal params() As SqlParameter) As DataSet
        Dim db As Database = DatabaseFactory.CreateDatabase()
        Dim ds As New DataSet
        Dim i As Integer

        Using cmd As DbCommand = db.GetStoredProcCommand(Sproc)
            cmd.CommandTimeout = 120

            For i = 0 To params.Length - 1
                db.AddInParameter(cmd, params(i).ParameterName.ToString, params(i).DbType, params(i).Value)
            Next

            db.LoadDataSet(cmd, ds, "table")
        End Using


        Return ds
    End Function

Could this error also mean that it is taking too long to possibly execute some queries? Some tables in my database have alot of data in them.

Any feedback or help would be greatly appreciated with this folks as it is really affecting my application.

Thanks.

Answer 1

As the connection handling appears to be in your Database class, I cannot be 100% sure as to the problem, though I can take a guess from your query method.

Strictly speaking, IDbConnection is IDisposable, so you should be disposing the connection. The underlying framework for SqlConnection implements its dispose method to instead return the connection to the pool.

If you do not call Close or Dispose on the connection, it will never free back up (see the SqlConnection MSDN page).

As an aside, you may consider re-working how you handle the database objects. I would suggest utilizing the ADO.NET interfaces rather than the System.Data.SqlClient objects directly, as this will decouple your code from SQL and make it far easier to test.

If you updated your Database class to return IDbConnection, every call in the method above could focus on the ADO.NET interfaces. Alternatively, you could continue to handle all this in your Database class. Additionally, you would want to either change your params parameter to IDataParameter or some other non-SQL type.

The workflow can go as follows (C# as I am more comfortable with the syntax):

IDbConnection conn = db.GetConnection ();
using (IDbCommand cmd = conn.CreateConnection ())
{
   cmd.CommandTimeout = 120;
   cmd.CommandText = strSproc;
   cmd.CommandType = CommandType.StoredProcedure;

   foreach (var param in params) // note: params is a reserved word
   {
      var  par = cmd.CreateParameter ();
      par.ParameterName = param.ParameterName;
      par.Value = param.Value;
      cmd.Parameters.Add (par);
   }

   var  retPar = cmd.CreateParameter ();
   retPar.ParameterName = "return_value";
   retPar.Direction = ParameterDirection.ReturnValue;

   cmd.ExecuteNonQuery ();

   var  retVal = (int) cmd.Parameters ["return_value"].Value;
}

For the sake of the example, I assumed that the parameters passed were IDataParameter, but I treated them as if they could not be used directly, since the interface-driven approach strongly pushes you to use the IDbCommand.CreateParameter method.

Answer 2

ExecuteReader passes out an open DataReader - the caller must take care to Dispose of that properly.

Most of your other SqlConnection is hidden in your DatabaseFactory.CreateDatabase(), which would need to take care of calling Dispose on those connections.

I note that ExecuteDataSet also instantiates it's own DatabaseFactory.CreateDatabase instead of using the class one. Not necessarily a problem, but odd and if DatabaseFactory leaks a connection, it'd be twice as bad.