1

I have this below code to retrieve data from db and when I run the Code Analysis from Visual Studio, it suggests me to call dispose method on SqlConnection, DataTable and SqlDataAdapter objects.

        SqlConnection sqlconn = new SqlConnection(ConfigurationManager.ConnectionStrings["connstr"].ConnectionString);
        SqlCommand cmd = sqlconn.CreateCommand();
        cmd.CommandText = "SELECT * FROM tbl_serial WHERE serial = @serial";
        cmd.Parameters.AddWithValue("@serial", txtQuery.Text);
        DataTable dt = new DataTable();
        SqlDataAdapter da = new SqlDataAdapter();
        try
        {
            sqlconn.Open();
            da.SelectCommand = cmd;
            da.Fill(dt);

        }
        catch (SqlException ex)
        {
            lblStatus.Text = ex.Message;
        }
        finally
        {
            sqlconn.Close();
        }

        if (dt.Rows.Count > 0)
        {
            lblStatus.Text = "FOUND!";
        }
        else
        {
            lblStatus.Text = "NOT FOUND!";
        }

And this is my first time doing this, I called dispose method on sqlconn just like this -

        finally
        {
            sqlconn.Close();
            sqlconn.Dispose();
        }

But then, Visual Studio warns me like this -

CA2202 : Microsoft.Usage : Object 'sqlconn' can be disposed more than once in method 'test_search.Unnamed1_Click(object, EventArgs)'. To avoid generating a System.ObjectDisposedException you should not call Dispose more than one time on an object.: Lines: 41

So I would like to know when should I correctly call dispose methods.

Ye Myat Aung
  • 1,783
  • 11
  • 31
  • 49

8 Answers8

6

Rather than calling dispose directly, use the using statement, which will make certain to do it for you when the code execution moves out of its associated block. E.g.:

SqlConnection con;

using (con = new SqlConnection(/* ... */)) {
    // Do your stuff with `con`
}

This is the pattern to use with disposables in general.

T.J. Crowder
  • 1,031,962
  • 187
  • 1,923
  • 1,875
  • 1
    he is using `finally`. isnt it enough? I am newbie please explain – Mr_Green Oct 29 '12 at 11:18
  • @Mr_Green: No, `finally` and `using` are completely different things. `finally` has nothing to do with the `IDisposable` interface. All that `finally` does is ensure that a group of statements is run regardless of whether the `try` block ends because it runs to completion or because of an exception. It doesn't do anything related to `dispose`. – T.J. Crowder Oct 29 '12 at 11:25
3

Here is your code re-written without ever having to all Dispose/Close manually

using (var sqlconn = new SqlConnection(ConfigurationManager.ConnectionStrings["connstr"].ConnectionString))
using (var cmd = sqlconn.CreateCommand())
{
    cmd.CommandText = "SELECT * FROM tbl_serial WHERE serial = @serial";
    cmd.Parameters.AddWithValue("@serial", txtQuery.Text);
    using (var dt = new DataTable())
    using (var da = new SqlDataAdapter())
    {
        try
        {
            sqlconn.Open();
            da.SelectCommand = cmd;
            da.Fill(dt);

        }
        catch (SqlException ex)
        {
            lblStatus.Text = ex.Message;
        }

        lblStatus.Text = dt.Rows.Count > 0 ? "FOUND!" : "NOT FOUND!";
    } 
}
James
  • 80,725
  • 18
  • 167
  • 237
  • Thanks for the code samples. Could you tell me what is the main difference in terms of advantages in using 'var' instead of the method I use to declare objects? – Ye Myat Aung Oct 29 '12 at 11:22
  • @YeMyatAung `var` is a compiler feature which allows *implicit declarations* i.e. the declaration type of the variable is inferred by the type of the value being assigned to it. It just means you don't have to write full class names everywhere. It was introduced in C# 3.0 so unless you are using a version below this you would need to stick with the long class names (you haven's stated which version of .NET your using). – James Oct 29 '12 at 11:24
  • Thanks for the explanation. I'm using .NET 4.0. – Ye Myat Aung Oct 29 '12 at 11:34
2

Best way to handle connection disposing is USING

So your code will become like this,

  using(SqlConnection sqlconn = new SqlConnection(ConfigurationManager.ConnectionStrings["connstr"].ConnectionString))
  { 
      code here..
  }

since Sqlconnection implements IDisposable, using block will automatically dispose object after call completes.

indiPy
  • 7,844
  • 3
  • 28
  • 39
1

sqlconn.Dispose() is just redundant in that finally block.A Close() in effect has called Dispose() and that's why the specific call to Dispose is throwing the error.

There are subtle differences between Close and Dispose.

  • Calling a close multiple times will not throw Exception.
  • Calling a dispose more than once will throw Exception.

In your case its very clear that the close has in turn invoked the Dispose. Your second call to Dispose therefore is causing the exception.

Always use USING as its the better approach.

Community
  • 1
  • 1
Prabhu Murthy
  • 9,031
  • 5
  • 29
  • 36
1
using(SqlConnection sqlconn = new SqlConnection(ConnectionStrings))
{ 
   //Automatically this block will disposes the SqlConnection
}

or 

you can dispose all your objects in Finally Block
andy
  • 5,979
  • 2
  • 27
  • 49
1

I agree with the others on using it definitely is best practice to use this. However, you should change your finally block to this:

finally
{
    if (sqlconn != null)
    {
        if (sqlConn.ConnectionState == ConnectionState.Open) sqlconn.Close();
        sqlConn.Dispose();
        GC.SuppressFinalize(sqlConn);
        sqlConn = null;
    }
}

With GC.SuppressFinalize you are telling the garbage collector to not bother disposing of sqlConn since you've disposed of it already. While it is possible to do this, I've always believed best practice is to implement IDisposable on your object and handle all clean ups in the Dispose method of the contract - also calling GC.SuppressFinalize(this) on your object.

public class MyObject : IDisposable
{
    private SqlConnection _sqlConn;

    public MyObject()
    {
        _sqlConn = new SqlConnection("connection string");
        _sqlConn.Open();
    }

    void IDisposable.Dispose()
    {
        if (_sqlConn != null)
        {
            if (_sqlConn.ConnectionState == ConnectionState.Open)
            {
                _sqlConn.Close();
            }

            _sqlConn.Dispose();
            _sqlConn = null;
        }

        // tell the garbage collector to ignore this object as you've tidied it up yourself
        GC.SuppressFinalize(this);
    }
}

This previous SO post has the best answer to this question.

Community
  • 1
  • 1
Paul Aldred-Bann
  • 5,840
  • 4
  • 36
  • 55
1

Hi it is good to write dispose and close statment in finaly as it will excute if there is exception occurd in the code.

finally
{
   sqlconn.Close();
   SqlConnection.ClearAllPool();  // this statement clears pool
   sqlConn.Dispose(); 
}

when you close or dispose connection it will not close connection at same time. it will only get close or cleared connection pool only when appdomain is refreshed

RVD
  • 66
  • 1
0

You probably want to format your code like this:

using (var sqlconn = new SqlConnection(...)){
  try {
            sqlconn.Open();
            da.SelectCommand = cmd;
            da.Fill(dt);
  }
  catch (SqlException ex) {
            lblStatus.Text = ex.Message;
  }
}

    if (dt.Rows.Count > 0)
    {
        lblStatus.Text = "FOUND!";
    }
    else
    {
        lblStatus.Text = "NOT FOUND!";
    }

This way, you let the using syntax take care of disposing/closing your connection and your code focuses on the logic that's important to you.

David Hoerster
  • 28,421
  • 8
  • 67
  • 102