Am I closing this SQL connection in a C# function appropriately?

Question

In an attempt to close my question on connections remaining open and exceeding the maximum pool, I'm trying tor rewrite the function that is used to connect to our database.

The function exists within a homegrown compiled library. using reflector I can see the code looks like this:

public SqlProvider([Optional, DefaultParameterValue("")] string StrConnection)
{
    string str;
    if (StrConnection == "")
    {
        str = ConfigurationSettings.AppSettings["ConStr"];
    }
    else
    {
        str = StrConnection;
    }
    SqlConnection connection = new SqlConnection(str);
    connection.Open();
    this.MyCommand = new SqlCommand();
    SqlCommand myCommand = this.MyCommand;
    myCommand.Connection = connection;
    myCommand.CommandType = CommandType.Text;
    myCommand = null;
    this.MyDataAdapter = new SqlDataAdapter(this.MyCommand);
    this.MyCommandBuilder = new SqlCommandBuilder(this.MyDataAdapter);
    this.MyDataSet = new DataSet();
}

I'm planning on amending this to read

public SqlProvider([Optional, DefaultParameterValue("")] string StrConnection)
{
    string str;
    if (StrConnection == "")
    {
        str = ConfigurationSettings.AppSettings["ConStr"];
    }
    else
    {
        str = StrConnection;
    }

    using (SqlConnection connection = new SqlConnection(str))
    {
        connection.Open();
        this.MyCommand = new SqlCommand();
        SqlCommand myCommand = this.MyCommand;
        myCommand.Connection = connection;
        myCommand.CommandType = CommandType.Text;
        myCommand = null;
        this.MyDataAdapter = new SqlDataAdapter(this.MyCommand);
        this.MyCommandBuilder = new SqlCommandBuilder(this.MyDataAdapter);
        this.MyDataSet = new DataSet();
    }
}

and then recompiling the dll. Given that an instance of SQLProvider() is typically created at the top of a public class, and then that instance is used within class members (eg:

public class Banner
{
    DSLibrary.DataProviders.SqlProvider db = new DSLibrary.DataProviders.SqlProvider(Defaults.ConnStr);
    public Banner()
    {    
    }

    public DataTable GetBannerImages(string bannerLocation,int DeptId)
    {
        using (DSLibrary.DataProviders.SqlProvider db = new DSLibrary.DataProviders.SqlProvider(Defaults.ConnStr))
        {
            DataTable dt = new DataTable();

            //Add Parameter @BannerLocation for Banner of Specific Location 
            //Call proc_getBannerImages Stored procedure for Banner Images
            db.AddStoredProcParameter("@BannerLocation", SqlDbType.VarChar, ParameterDirection.Input, 100, bannerLocation);
            db.AddStoredProcParameter("@DeptId", SqlDbType.Int, ParameterDirection.Input, 0, DeptId);
            dt = db.ExecuteStoredProcedure("proc_getBannerImages");
            return dt;
        }
    }
}

am I going about this the right way? It seems to me the connection will be disposed of before the data has actually been retrieved. Also, Visual Studio tells me that SQLProvider() must be implicitly convertible to System.IDisposable - how would I go about implementing this?

I tried wrapping all the members of class Banner in a using (DSLibrary.DataProviders.SqlProvider db = new DSLibrary.DataProviders.SqlProvider(Defaults.ConnStr)){} statement but intellisense then displays a "Invalid token 'using' in class, struct, or interface member declaration" error.

What is the best way to go about this?

UPDATE I've tried disassembling adjusting and recompiling the DSLibrary, but as CHris_Lively says, I thinkit's doing nothing for me. Changing the instance in question to what I preceive to be a more standard format works so far:

public DataTable GetBannerImages(string bannerLocation,int DeptId)
{
    using (SqlConnection conn = new SqlConnection(Defaults.ConnStr))
    {
        SqlCommand cmd = new SqlCommand("proc_getBannerImages", conn);
        cmd.CommandType = CommandType.StoredProcedure;
        cmd.Parameters.Add(new SqlParameter("@BannerLocation", bannerLocation));
        cmd.Parameters.Add(new SqlParameter("@DeptId", DeptId));

        SqlDataAdapter da = new SqlDataAdapter();
        da.SelectCommand = cmd;
        DataTable dt = new DataTable();

        da.Fill(dt);
        return dt;
    }
}

I'm about to look into the Enterprise library, seems like it might be the way forward.

Answer 1

There is no need to set variables to null - anyway they will be deleted by GC.

Also you need to call Dispose() or Close() for all classes that implements IDisposable. e.g. SqlConnection.

You can do that manually:

SqlConnection conn = null;
try
{
    // use conn
}
finally
{
    if (conn != null)
        conn.Close();
}

or automatically using using block:

using (SqlConnection = new SqlConnection())
{
    // use conn
}

(as you do)

Also you can decrease your code a bit using operator ?:

string str = String.IsNullOrEmpty(StrConnection) ? ConfigurationSettings.AppSettings["ConStr"] : StrConnection;

or ??:

string str = StrConnection ?? ConfigurationSettings.AppSettings["ConStr"];

Answer 2

I DO NOT think you are doing it correctly. As soon as you hit the end of the using block, the SqlConnection variable will become unusable . If you want to use it outside the constructor, dont put the using {} around the SqlConnection variable (The sqlcommand variable MyCommand is using it indirectly outside the constructor).
Instead, make you SqlProvider class implement IDisposable, and call Dispose on the MyCommand, MyDataAdapter, MyDataSet etc. variables there.
You probably should have something like this in your SqlProvider class :

    public void Dispose()
    {
        if (MyCommand != null)
        {
            MyCommand.Dispose();
        }
        //... Similarly for MyDataAdapter,MyDataSet etc.
    }

Your class needs to implement the IDisposable interface if you want to use it in a using block. See http://msdn.microsoft.com/en-us/library/system.idisposable.dispose%28v=VS.100%29.aspx for guidelines on dispose() and IDisposable.

Answer 3

It is not recommended to hold on to connections any longer than required (See also Chapter 14 of Improving .NET Application Performance and Scalability: Patterns and Practices from Microsoft Press).

In practice I'd change your class to instead not have the SqlConnection (or SqlDataAdapter or SqlCommandBuilder) as a member on the class (if you must, then you should implement the IDisposable pattern), but instead create new instances, wrapped in using statements on the class methods that need to use them.

Answer 4

You're close. However, a couple issues.

First, it looks like that whole DSLibrary isn't buying you anything at all.

When doing data access you typically want to structure it where acquiring the connection and executing the command are in the same function. You're methods should only return the result of the operation. This way you can cleanly use the IDisposable interface of the connection, command, and reader.

The following example uses Enterprise Library. Note that the Db doesn't have a using clause. It doesn't implement IDisposable. Instead, the command is responsible for letting go of the connection when it goes out of scope:

    public static DataTable GetBannerImages(String bannerLocation, Int32 departmentId)
    {
        DataTable result = new DataTable();
        result.Locale = CultureInfo.CurrentCulture;

        Database db = DatabaseFactory.CreateDatabase("NamedConnectionStringFromConfig");

        using (DbCommand dbCommand = db.GetStoredProcCommand("proc_getBannerImages"))
        {
            db.AddInParameter(dbCommand, "BannerLocation", DbType.String, bannerLocation);
            db.AddInParameter(dbCommand, "DeptId", DbType.Int32, departmentId);

            using (IDataReader reader = db.ExecuteReader(dbCommand))
            {
                SopDataAdapter dta = new SopDataAdapter(); // descended from DbDataAdapter

                dta.FillFromReader(result, reader);
            } // using dataReader
        } // using dbCommand

        return result;
    } // method::GetBannerImages

You probably already have something that will convert a reader to a datatable, if not just look into subclassing the System.Data.Common.DbDataAdapter class

I've had tremendous success with the Enterprise Library. It's fast, efficient, and when going this route I've never had memory leaks or db connection issues.