CODEWITHSUNDEEP
c#.netsqlsqlconnection
Eric Schoonover
Eric Schoonover

Reputation: 48402

What is the proper way to ensure a SQL connection is closed when an exception is thrown?

I use a pattern that looks something like this often. I'm wondering if this is alright or if there is a best practice that I am not applying here.

Specifically I'm wondering; in the case that an exception is thrown is the code that I have in the finally block enough to ensure that the connection is closed appropriately?

public class SomeDataClass : IDisposable
{
    private SqlConnection _conn;

    //constructors and methods

    private DoSomethingWithTheSqlConnection()
    {
        //some code excluded for brevity

        try
        {
            using (SqlCommand cmd = new SqlCommand(SqlQuery.CountSomething, _SqlConnection))
            {
                _SqlConnection.Open();
                countOfSomething = Convert.ToInt32(cmd.ExecuteScalar());
            }
        }
        finally
        {
            //is this the best way?
            if (_SqlConnection.State == ConnectionState.Closed)
                _SqlConnection.Close();
        }

        //some code excluded for brevity
    }

    public Dispose()
    {
        _conn.Dispose();
    }
}

Upvotes: 18

Views: 18686

Answers (9)

Jeffrey L Whitledge
Jeffrey L Whitledge

Reputation: 59463

I'm guessing that by _SqlConnection.State == ConnectionState.Closed you meant !=

This will certainly work. I think it is more customary to contain the connection object itself inside a using statement, but what you have is good if you want to reuse the same connection object for some reason.

One thing that you should definitely change, though, is the Dispose() method. You should not reference the connection object in dispose, because it may have already been finalized at that point. You should follow the recommended Dispose pattern instead.

Upvotes: 2

Cyber Oliveira
Cyber Oliveira

Reputation: 8596

The .Net Framework mantains a connection pool for a reason. Trust it! :) You don't have to write so much code just to connect to the database and release the connection.

You can just use the 'using' statement and rest assured that 'IDBConnection.Release()' will close the connection for you.

Highly elaborate 'solutions' tend to result in buggy code. Simple is better.

Upvotes: 8

BlackTigerX
BlackTigerX

Reputation: 6146

no need for a try..finally around a "using", the using IS a try..finally

Upvotes: 0

Ed Schwehm
Ed Schwehm

Reputation: 2171

Put the connection close code inside a "Finally" block like you show. Finally blocks are executed before the exception is thrown. Using a "using" block works just as well, but I find the explicit "Finally" method more clear.

Using statements are old hat to many developers, but younger developers might not know that off hand.

Upvotes: 1

Brannon
Brannon

Reputation: 26109

See this question for the answer:

Close and Dispose - which to call?

If your connection lifetime is a single method call, use the using feature of the language to ensure the proper clean-up of the connection. While a try/finally block is functionally the same, it requires more code and IMO is less readable. There is no need to check the state of the connection, you can call Dispose regardless and it will handle cleaning-up the connection.

If your connection lifetime corresponds to the lifetime of a containing class, then implement IDisposable and clean-up the connection in Dispose.

Upvotes: 1

Amy B
Amy B

Reputation: 110111

MSDN Docs make this pretty clear...

  • The Close method rolls back any pending transactions. It then releases the connection to the connection pool, or closes the connection if connection pooling is disabled.

You probably haven't (and don't want to) disable connection pooling, so the pool ultimately manages the state of the connection after you call "Close". This could be important as you may be confused looking from the database server side at all the open connections.

  • An application can call Close more than one time. No exception is generated.

So why bother testing for Closed? Just call Close().

  • Close and Dispose are functionally equivalent.

This is why a using block results in a closed connection. using calls Dispose for you.

  • Do not call Close or Dispose on a Connection, a DataReader, or any other managed object in the Finalize method of your class.

Important safety tip. Thanks, Egon.

Upvotes: 6

Torbjörn Gyllebring
Torbjörn Gyllebring

Reputation: 18228

Might I suggest this:


    class SqlOpener : IDisposable
    {
        SqlConnection _connection;

        public SqlOpener(SqlConnection connection)
        {
            _connection = connection;
            _connection.Open();

        }

        void IDisposable.Dispose()
        {
            _connection.Close();
        }
    }

    public class SomeDataClass : IDisposable
    {
        private SqlConnection _conn;

        //constructors and methods

        private void DoSomethingWithTheSqlConnection()
        {
            //some code excluded for brevity
            using (SqlCommand cmd = new SqlCommand("some sql query", _conn))
            using(new SqlOpener(_conn))
            {
                int countOfSomething = Convert.ToInt32(cmd.ExecuteScalar());
            }
            //some code excluded for brevity
        }

        public void Dispose()
        {
            _conn.Dispose();
        }
    }

Hope that helps :)

Upvotes: -4

Aaron Maenpaa
Aaron Maenpaa

Reputation: 122900

Since you're using IDisposables anyway. You can use the 'using' keyword, which is basically equivalent to calling dispose in a finally block, but it looks better.

Upvotes: 1

Sklivvz
Sklivvz

Reputation: 31133

Wrap your database handling code inside a "using"

using (SqlConnection conn = new SqlConnection (...))
{
    // Whatever happens in here, the connection is 
    // disposed of (closed) at the end.
}

Upvotes: 46

Related Questions