Just how bad is it not to Dispose() SqlConnections?

Question

Personally, I break out in hives if I don't put ADO objects that implement IDisposable in using statements. But at my current contract, I've found that their homegrown enterprise framework "data access provider" code does not 1) implement IDisposable and 2) call Dispose() on anything it uses, at any point, ever. Users have been complaining a great deal about performance issues in the Winforms applications that heavily use this framework for data access, and though there are a LOT of other problems in the code that could be hitting performance, this one just screams at me and is more low-hanging-fruit than the others.

So, beyond saying something like "Dispose is there for a reason, use it," what can I tell these people to convince them that this is really, really bad?

Answer 1

I'd say the best thing you can do is point them to Microsoft's Patterns and Practices regarding Dispose() here. Let them see the full consequences of not utilizing this tool that's right in front of them.

Answer 2

If you do not call the Dispose method on a SQL Connection, when you done using it, that connection does NOT get returned back to the connection pool.

If you are having performance issues, my guess is that the maximum connections are opened on your database. A DBA could easily confirm this.

Microsoft's Best Practice calls for you to place your connection code inside of a Using statement, ensuring the connection is disposed of and that the connection is returned back to the pool.

Answer 3

The database connection pool is limited in size, and if it fills up, new connections will wait for old connections to be released. If you don't dispose them as soon as you're finished with them, they'll eventually get released when the finaliser runs, but that's an indeterminate amount of time in the future... So at best, you're going to see long delays when opening new connections.

At worst, they might've disabled the connection pool. Perhaps they discovered that after a while their app was returning "timeout waiting for connection" errors and disabling the connection pool "solved" that problem. However, this is much worse because it means you're creating a whole new connection every time - which is surprisingly resource intensive. If you've got hundreds of connections to the database open, it would be no wonder you're seeing performance problems.

The correct way to solve all of these problems is to dispose of your connection as soon as you're finished with it. The best idea is to hold the connection open for exactly as long as needed, and no longer.

Answer 4

In any serious application I would say that it is pretty bad. Not only will leaving these objects float around kill performance, it is also simply unprofessional. The good news is that Microsoft implements Finalize in the object hierarchy.

~Object()
{
    this.Dispose(false);    
}

public void Dispose()
{
    this.Dispose(true);
    GC.SuppressFinalize(this);
}

protected virtual void Dispose(bool disposing)
{
    // ...
}

Take System.Data.SqlClient.SqlConnection for example:

System.ComponentModel.Component <-- Implements the Finalize dispose pattern.
    |
System.Data.Common.DbConnection
    |
System.Data.SqlClient.SqlConnection

The objects will eventually be disposed but the non-deterministic nature reaks havoc on performance.

Answer 5

Well, for one, you aren't terminating the connection. So it either has to a) Be dropped automatically or b) Get cycled through and updated even though the client has no use for it.

I'd assume b) due to the performance hit you're describing. However, that is likely not the only reason.

You MUST close your connections, preferably on the client side, but you must also implement a fail safe on the server side. Otherwise you've just got extra crap piled on that your database server has to process until it gets released at god-knows-when.