134

I have my business-logic implemented in simple static classes with static methods. Each of these methods opens/closes SQL connection when called:

public static void DoSomething()
{
    using (SqlConnection connection = new SqlConnection("..."))
    {
        connection.Open();

        // ...

        connection.Close();
    }
}

But I think passing the connection object around and avoiding opening and closing a connection saves performance. I made some tests long time ago with OleDbConnection class (not sure about SqlConnection), and it definitely helped to work like this (as far as I remember):

//pass around the connection object into the method
public static void DoSomething(SqlConnection connection)
{
    bool openConn = (connection.State == ConnectionState.Open);
    if (!openConn)
    {
        connection.Open();
    }

    // ....

    if (openConn) 
    {
        connection.Close();
    }
}

So the question is - should I choose the method (a) or method (b) ? I read in another stackoverflow question that connection pooling saved performance for me, I don't have to bother at all...

PS. It's an ASP.NET app - connections exist only during a web-request. Not a win-app or service.

Alex from Jitbit
  • 53,710
  • 19
  • 160
  • 149
  • 1
    Just an advise: Use `DbConnection.StateChange` event to monitor changes in connection's state change (and may be store locally) instead of checking `DbConnection.State` property directly. It will save you performance cost. – decyclone Dec 14 '10 at 13:17
  • 1
    One detail that is missing is how this method is part of a page request. Is it the only method called or is it, as I assumed in my response, one of many methods thats is called in a page reqest, it affects which answer is right ;) – David Mårtensson Dec 14 '10 at 13:24
  • David - LOTS of methods like this are being called:) – Alex from Jitbit Dec 14 '10 at 17:49
  • 1
    Case A shows a lack of belief in Dispose: see http://stackoverflow.com/questions/1195829/do-i-have-to-close-a-sqlconnection-before-it-gets-disposed and the example on MSDN https://msdn.microsoft.com/en-us/library/system.data.sqlclient.sqlconnection.open(v=vs.110).aspx – user2864740 Oct 23 '16 at 20:14

6 Answers6

92

Stick to option a.

The connection pooling is your friend.

Adriaan Stander
  • 162,879
  • 31
  • 289
  • 284
  • 45
    IMHO - he should'nt even do close. the dispose will do it. – Royi Namir Jan 07 '13 at 20:11
  • 2
    @RoyiNamir I kinda like the call to close the connection. Especially for beginners and newcomers to a code base. It's more explicit and readable. – edhedges Jul 25 '14 at 19:39
  • 34
    @edhedges Utilizing both "using" and Close() is ultimately only going to cause confusion for newcomers. They are not going to understand the purpose of utilizing "using". Don't use "Close" instead teach them the purpose of "using". So that they can learn get better and apply what they learn to other parts of the code. – Luis Perez Oct 09 '15 at 12:51
  • 1
    Should/does the "Open()" need to be called? I am currently using it like this: using (var conn = GetConnection()) {} public SqlConnection GetConnection() { return new SqlConnection(_connectionString); } – ganders Jan 23 '20 at 17:12
82

Use Method (a), every time. When you start scaling your application, the logic that deals with the state will become a real pain if you do not.

Connection pooling does what it says on the tin. Just think of what happens when the application scales, and how hard would it be to manually manage the connection open/close state. The connection pool does a fine job of automatically handling this. If you're worried about performance think about some sort of memory cache mechanism so that nothing gets blocked.

WeNeedAnswers
  • 4,620
  • 2
  • 32
  • 47
36

Always close connections as soon as you are done with them, so they underlying database connection can go back into the pool and be available for other callers. Connection pooling is pretty well optimised, so there's no noticeable penalty for doing so. The advice is basically the same as for transactions - keep them short and close when you're done.

It gets more complicated if you're running into MSDTC issues by using a single transaction around code that uses multiple connections, in which case you actually do have to share the connection object and only close it once the transaction is done with.

However you're doing things by hand here, so you might want to investigate tools that manage connections for you, like DataSets, Linq to SQL, Entity Framework or NHibernate.

Neil Barnwell
  • 41,080
  • 29
  • 148
  • 220
  • You should not normaly open and close a connection within every method call, only once for each page request. Thats what I have learned at least ;) Opening and closing costs time. – David Mårtensson Dec 14 '10 at 13:11
  • 11
    @David Martensson - connections are not actually opened and closed when you call SqlConnection.Open. ASP.NET recycles active connections from the pool when the connection string matches a previously used connection string. The overhead involved in this is inconsequential, and additionally, trying to "do it yourself" means you have to assume all the management tasks of ensuring the connection is still active for each subsequent use, which adds complexity and overhead. With connection pooling, best practice is to open and close it for every use. – Jamie Treworgy Dec 14 '10 at 13:23
  • 2
    With all my respect, the answer "Always close connections" does not fit the question very well... I do close them. The question is - when. – Alex from Jitbit Dec 14 '10 at 17:52
  • @David Martensson "Once for each page" is oversimplified. You're right that if you have several database commands to execute one after the other, you can keep the connection open while you execute them. There would be a minor overhead if you closed and reopened - the connection would go into the pool and be retrieved from it a moment later. – Concrete Gannet Mar 31 '17 at 03:03
  • 1
    @David Martensson But never keep an idle connection. If you are waiting for an action from the user or for anything else, close it. If in doubt, close it. You open as late as you can in the hope somebody else has finished with a connection and pooled it. Then you return the favour - close as early as you reasonably can. – Concrete Gannet Mar 31 '17 at 03:05
  • @ConcreteGannet The question specifically stated it is an ASP.net page which means that during page rendering you will never get any extra user input. A request consists of a get or post to the server followed by a reply and most page requests take less than a second to complete. If you have so much pressure on the site that keeping the connection for 300-500 ms then I would recommend looking at caching through memcached or redis to eliminate sql queries as much as possible. – David Mårtensson Apr 03 '17 at 12:08
14

Disclaimer: I know this is old, but I found an easy way to demonstrate this fact, so I'm putting in my two cents worth.

If you're having trouble believing that the pooling is really going to be faster, then give this a try:

Add the following somewhere:

using System.Diagnostics;
public static class TestExtensions
{
    public static void TimedOpen(this SqlConnection conn)
    {
        Stopwatch sw = Stopwatch.StartNew();
        conn.Open();
        Console.WriteLine(sw.Elapsed);
    }
}

Now replace all calls to Open() with TimedOpen() and run your program. Now, for each distinct connection string you have, the console (output) window will have a single long running open, and a bunch of very fast opens.

If you want to label them you can add new StackTrace(true).GetFrame(1) + to the call to WriteLine.

Chris Pfohl
  • 18,220
  • 9
  • 68
  • 111
11

There are distinctions between physical and logical connections. DbConnection is a kind of logical connection and it uses underlying physical connection to Oracle. Closing/opening DbConnection doesn't affect your performance, but makes your code clean and stable - connection leaks are impossible in this case.

Also you should remember about cases when there are limitations for parallel connections on db server - taking that into account it is necessary to make your connections very short.

Connection pool frees you from connection state checking - just open, use and immediately close them.

Tony Kh
  • 1,512
  • 11
  • 8
  • Yes, the connection is not the connection - i.e. DbConnection is not the physical connection. DbConnection is a .NET class that provides methods and properties to manipulate the underlying physical connection. – Concrete Gannet Mar 31 '17 at 03:07
  • Unfortunately it wasn't immediately obvious that this was done all implicitly, but the documentation elaborates on it. https://learn.microsoft.com/en-us/dotnet/framework/data/adonet/sql-server-connection-pooling – Austin Salgat Apr 28 '20 at 05:56
1

Normally you should keep one connect for each transaction(no parallel computes)

e.g when user execute charge action, your application need find user's balance first and update it, they should use same connection.

Even if ado.net has its connection pool, dispatching connection cost is very low, but reuse connection is more better choice.

Why not keep only one connection in application

Because the connection is blocking when you execute some query or command, so that means your application is only doing one db operation at sametime, how poor performance it is.

One more issue is that your application will always have a connection even though your user is just open it but no operations.If there are many user open your application, db server will cost all of its connection source in soon while your users have not did anything.

ShiningRush
  • 41
  • 5