Why Connection not being closed in this code

Question

I am maintaining a previous developer work.

Here is the db layer class, it has .......

public class Database 
{
        private string mConnString;
        private SqlConnection mConn;
        private SqlDataAdapter mAdapter;
        private SqlCommand mCmd;
        private SqlTransaction mTransaction;
        private bool disposed = false;

        public Database() : this(Web.GetWebConfigValue("ConnectionString"))
        {
        }

        public Database(string connString)
        {
            mConnString = connString;
            mConn = new SqlConnection(mConnString);
            mConn.Open();
            mAdapter = new SqlDataAdapter();
            mCmd = new SqlCommand();
            mCmd.CommandType = CommandType.StoredProcedure;
            mCmd.Connection = mConn;
        }

        public void CloseConnection()
        {
            mConn.Close();
        }

        public void BeginTransaction()
        {
            mTransaction = mConn.BeginTransaction();
            mCmd.Transaction = mTransaction;
        }

        public void CommitTransaction()
        {
            mTransaction.Commit();
        }

        public void RollbackTransaction()
        {
            mTransaction.Rollback();
        }

        public void AddParam(string name, SqlDbType type, object value)
        {
            SqlParameter parameter = new SqlParameter('@' + name, type);
            parameter.Value = value;
            mCmd.Parameters.Add(parameter);
        }

        public void ChangeParam(string name, object value)
        {
            mCmd.Parameters['@' + name].Value = value;
        }

        public void DeleteParam(string name)
        {
            mCmd.Parameters.RemoveAt('@' + name);
        }

        public void AddReturnParam()
        {
            SqlParameter parameter = new SqlParameter();
            parameter.ParameterName = "return";
            parameter.Direction = ParameterDirection.ReturnValue;
            mCmd.Parameters.Add(parameter);
        }

        public void AddOutputParam(string name, SqlDbType type, int size)
        {
            SqlParameter parameter = new SqlParameter('@' + name, type);
            parameter.Direction = ParameterDirection.Output;
            parameter.Size = size;
            mCmd.Parameters.Add(parameter);
        }

        public int GetReturnParam()
        {
            return (int)mCmd.Parameters["return"].Value;
        }

        public object GetOutputParam(string name)
        {
            return mCmd.Parameters['@' + name].Value;
        }

        public void ClearParams()
        {
            mCmd.Parameters.Clear();
        }

        public void ExecNonQuery(string cmdText)
        {
            if(mConn.State==ConnectionState.Closed)
                mConn.Open();

            mCmd.CommandText = cmdText;
            mCmd.ExecuteNonQuery();
        }

        public DataSet GetDataSet(string cmdText)
        {
            mCmd.CommandText = cmdText;
            mAdapter.SelectCommand = mCmd;
            DataSet ds = new DataSet();
            mAdapter.Fill(ds);
            return ds;
        }

        public IDataReader GetDataReader(string cmdText)
        {
            mCmd.CommandText = cmdText;

            if(mConn.State==ConnectionState.Closed)
                mConn.Open();

            return mCmd.ExecuteReader(CommandBehavior.CloseConnection);
        }

        public DataTable GetDataTable(string cmdText)
        {
            return GetDataSet(cmdText).Tables[0];
        }

        public DataTable GetDataTable(string cmdText,string SQL)
        { 
            SqlCommand cmd = new SqlCommand();
            cmd.CommandText = cmdText;
            mAdapter.SelectCommand = cmd;
            cmd.Connection = mConn;
            DataSet ds = new DataSet();
            mAdapter.Fill(ds);
            return ds.Tables[0];
        }

        public DataRow GetDataRow(string cmdText)
        {
            DataTable dt = GetDataTable(cmdText);
            DataRow dr;

            if(dt.Rows.Count > 0)
                dr = dt.Rows[0];
            else
                dr = null;

            return dr;
        }

        public object GetScalar(string cmdText)
        {
            mCmd.CommandText = cmdText;
            return mCmd.ExecuteScalar();
        }

        public void SetCommandType(CommandType type)
        {
            mCmd.CommandType = type;
        }

        ~Database()
        {
            this.Dispose(false);
        }
        public void Dispose()
        {
            Dispose(true);
            GC.SuppressFinalize(this);
        }
        private void Dispose(bool disposing)
        {
            if(!this.disposed)
            {
                if(disposing)
                {
                    if(mConn.State == ConnectionState.Open)
                        mConn.Close();
                    this.mCmd.Dispose();
                    this.mAdapter.Dispose();
                    this.mTransaction.Dispose();
                }
            }
            disposed = true;         
        }


    }

Can you help me in figuring out where connection might not be closed in all the cases where this class is being used.

Answer 1

You aren't implementing the disposable pattern via the IDisposable interface, you just have a Dispose method, in turn you would not be able to call this in a using statement.

public class Database : IDisposable { ... }

This is all a bit suspect: I mean, if you are already using it you aren't using this in a using statement and trying to cache the connection seemingly. I would shy away from this altogether.

Also you have a Destructor, however its usage is wrong 99% of times.

Answer 2

The connection only ever gets closed when disposing the instance of the DataBase class. However, while this class is implementing the disposable pattern, it doesn't implement the IDisposable interface - so you can't use it in a using statement.
Moreover, you have to rely on whoever is using this class to dispose it.
If they don't, the connection will not get closed until the Finalizer gets called, and that is completely out of the developer's control. It might even not get called at all - as the garbage collector might not need to clear memory during the runtime of whatever application is using this code.

This is why the proper way of handling connections to the database is as a local variable inside a using statement.

What you want to do is create the connection and open it as late as possible, and dispose it as soon as possible.
A proper method for handling calls to the database looks like this:

int ExecuteNonQuery(string sql)
{
    using(var con = new SqlConnection(connectionString))
    {
        using(var cmd = new SqlCommand(sql, con))
        {
            con.Open();
            return cmd.ExecueNonQuery();
        }
    }
}

Of course, you would want to add arguments to hold whatever parameters you'll need to pass to the database, and an argument to hold command type, but that should be built on the basis of this structure.

I have a project on GitHub called ADONETHelper (that I've been neglecting for the past year or so due to lack of spare time) that was written in order to reduce the code repetition when using ADO.Net directly.
I've written it a few years back so of course now I have improvements in mind but as I said, I don't have the spare time to work on it - but the general idea is still valid and useful. Basically, it has a single Execute method that looks like this:

public T Execute<T>(string sql, CommandType commandType, Func<IDbCommand, T> function, params IDbDataParameter[] parameters)
{
    using (var con = new TConnection())
    {
        con.ConnectionString = _ConnectionString;
        using (var cmd = new TCommand())
        {
            cmd.CommandText = sql;
            cmd.Connection = con;
            cmd.CommandType = commandType;
            if (parameters.Length > 0)
            {
                cmd.Parameters.AddRange(parameters);
            }
            con.Open();
            return function(cmd);
        }
    }
}

Than I've added a few methods that use this method:

public int ExecuteNonQuery(string sql, CommandType commandType, params IDbDataParameter[] parameters)
{
    return Execute<int>(sql, commandType, c => c.ExecuteNonQuery(), parameters);
}

public bool ExecuteReader(string sql, CommandType commandType, Func<IDataReader, bool> populate, params IDbDataParameter[] parameters)
{
    return Execute<bool>(sql, commandType, c => populate(c.ExecuteReader()), parameters);
}

and so on.

Feel free to borrow ideas from that project - or even use it as is - I have a few applications using this and they run very well for quite some time now.

Answer 3

While a persistence layer makes perfectly sense, you have to design it differently. What you do is pack some complexity into methods which still do the same, such as ChangeParam() or GetDataReader().

Normally, you have repositories which have knowledge about the underlying technicalities and remove that kind of complexity, such as GetAllCustomers() (with emphasis on the domain term customer).

When you have say 4 or 5 such repositories, then you start refactoring by abstracting complexity into a parent class. By doing so, you're packing the complexity such as in GetDataReader() and promote it from the repositories into an abstract repository that sits on top of it.

By starting from where you started you'll get just another layer which does not abstract nearly as much and has too much, often unnecessary, functionality.

If it were that easy, the ADO.NET API would already have it removed in the first place.

Another thing you should do is look at this simple but ever-recurring code snippet. It distillates some core concepts about IDisposable and using(){}. You'll always encounter it in correct code:

string sql = "SELECT * FROM t";

using (SqlConnection con = new SqlConnection(connectionString))
using (SqlCommand cmd = new SqlCommand(sql, con))
{
    SqlDataReader reader;

    con.Open();
    reader = cmd.ExecuteReader();

    while (reader.Read())
    {
        // TODO: consume data
    }
    reader.Close();
}

This is the stuff I'd expect to see in a persistence layer, and the consume data part is actually the most important, because it is domain-dependent. The rest is just boilerplate code with little interest.