DbDataReader with DbTransactions

Question

Its the wrong way or lack of performance, using DbDataReader combinated with DbTransactions? An example of code:

public DbDataReader ExecuteReader()
    {
        try
        {
            if (this._baseConnection.State == ConnectionState.Closed)
                this._baseConnection.Open();
            if (this._baseCommand.Transaction != null)
                return this._baseCommand.ExecuteReader();
            return this._baseCommand.ExecuteReader(CommandBehavior.CloseConnection);
        }
        catch (Exception excp)
        {
            if (this._baseCommand.Transaction != null)
                this._baseCommand.Transaction.Rollback();
            this._baseCommand.CommandText = string.Empty;
            this._baseConnection.Close();
            throw new Exception(excp.Message);
        }
    }

Some methods call this operation. Sometimes openning a DbTransaction. Its using DbConnection and DbCommand.

The real problem, is in production enviroment (like 5,000 access/day) the ADO operations start throwing exceptions

It has an method, that doesnt open a DbTransaction, but throws the excp anyway.

EDIT: We implemented log, to analyse ADO operations. This was an approach to catch the ADO problems in production enviroment. The exceptions caught were:

There is already an open DataReader associated with this Command which must be closed first.

Invalid attempt to call Read when reader is closed.

The connection was not closed. The connection's current state is open.

Also, we realized that the dbHelper class, is instantiated this way:

private static readonly dbHelper<T> _instance = new dbHelper<T>();

public static dbHelper<T> GetInstance()
{
    return _instance;
}

And the DAO's constructors, instatiate the dbHelper:

this._dataPersist = 
            Registro.Classes.dbHelper<System.Data.SqlClient.SqlClientFactory>.GetInstance();

We think changing the data access code, replacing the generic dbHelper with another approach, might fix the problem. Any suggestions would be appreciated.

Answer 1

One potential issue is that DbTransaction, DbConnection, and their ilk are all abstract classes. At some point you need to provide real concrete implementations of those types. This strikes me as something the compiler would catch,though, but I thought it was still worth mentioning.

More likely it's that you're leaving connections open for too long. Reading your bold sentence, I'm left with the impression that the production code does not throw exceptions initially. The exceptions only come after you've left it running a little while. That matches with the code you posted, too, because just in your small shared code I already see two places where your code doesn't clean up it's resources correctly.

Your execute reader function will not close the base connection if an exception is thrown while rolling back a transaction. Additionally, the snippet you posted at the end of your question may not properly close the reader. The correct way to dispose of your resources is with a using block, like this:

using (var reader = this._dataPersist.ExecuteReader(true))
{
    /* populate the obj here
    * */
}
return obj;

Note that there is no need to call the Close() or Dispose() method. The using block takes care of that for you, and it does it even if an exception is thrown.