views:

438

answers:

4

Hi everyone,

I'm currently using a singleton on my web application so that there is always only one connection to the database.

I want to know if it's a good idea because right now I'm having trouble with that error:

Timeout expired. The timeout period elapsed prior to obtaining a connection from the pool. This may have occurred because all pooled connections were in use and max pool size was reached.

Another important point is that my website is currently in dev and not a lot of people go on it so I don't understand why I get this error!

Here is the code of my singleton:

using System;
using System.Data;
using System.Configuration;
using System.Web;
using System.Web.Security;
using System.Web.UI;
using System.Web.UI.WebControls;
using System.Web.UI.WebControls.WebParts;
using System.Web.UI.HtmlControls;
using System.Data.SqlClient;

/// <summary>
/// This class take care of all the interaction with the database
/// </summary>
public class DatabaseFacade
{
    SqlConnection m_conn = null;

    string m_csLanguageColumn;

    //Variables that implement the Singleton pattern
    //Singleton pattern create only one instance of the class
    static DatabaseFacade instance = null;
    static readonly object padlock = new object();

    /// <summary>
    /// Private constructor. We must use Instance to use this class
    /// </summary>
    private DatabaseFacade()
    {
    }

    /// <summary>
    /// Static method to implement the Singleton
    /// </summary>
    public static DatabaseFacade Instance
    {
        get
        {
            lock (padlock)
            {
                if (instance == null)
                {
                    instance = new DatabaseFacade();
                }
                return instance;
            }
        }
    }

    /// <summary>
    /// Do the connection to the database
    /// </summary>
    public void InitConnection(int nLanguage)
    {
     m_conn = new SqlConnection(GetGoodConnectionString());

        try
        {
            //We check if the connection is not already open
            if (m_conn.State != ConnectionState.Open)
            {
                m_conn.Open();
            }

            m_csLanguageColumn = Tools.GetTranslationColumn(nLanguage);

        }
        catch (Exception err)
        {
            throw err;
        }
    }
}

Thanks for your help!

Jaff

+5  A: 

No, it's a bad idea. You use connection pooling.

Noon Silk
Couldn't have put it any better myself. :-P
jrista
An explanation would have been nice.
Robert Harvey
@Robert, agreed. Downvoted accordingly.
Nick Stinemates
Ah downvoters, I love you. Never give up! @Robert: I didn't explain further as there are better answers which I upvoted :)
Noon Silk
+10  A: 

Using a single connection is an extremely bad idea - if access to the connection is properly locked, it means that ASP.NET can only serve one user at a time, which will seriously limit your application's ability to grow.

If the connection is not properly locked, things can get really weird. For example, one thread might dispose the connection while another thread is trying to execute a command against it.

Instead of using a single connection, you should just create new connection objects when you need them, to take advantage of connection pooling.

Connection pooling is the default behavior for the SqlClient classes (and probably other data providers). When you use connection pooling, any time you 'create' a connection, the connection will actually be pulled from a pool of existing ones so that you don't incur the costs of building one from scratch each time. When you release it (close it or dispose of it) you return it to the connection pool, keeping your total count of connections relatively low.


Edit: You'll see the error you mention (The timeout period elapsed prior to obtaining a connection from the pool) if you're not closing (or disposing) your connections. Make sure you do that as soon as you're done using each connection.

There are several good stack overflow questions that discuss this, which I suspect might be helpful!

Jeff Sternal
While I agree that a singleton is a bad idea for SqlConnection, I don't see any indication that it'd be limited to a single request. There's no locking in the code that'd prevent multiple users of the SqlConnection object (which, if anything, just makes it that much worse of an idea because of threading issues).
Mark Brackett
@Mark, d'oh! I wrote that without reading through the entire code, assuming that there was locking restricting use of the Connection! As you note, that can cause a whole different class of problems. I've updated my answer to reflect this.
Jeff Sternal
Thanks guys! Very quick and clear answer. It will be easy to fix this! Thanks a lot again!
DarkJaff
+3  A: 

The reason why using a Connection to the database as a singleton is an horrific idea, is because every 2nd+ connection will then have to WAIT for the first connection to be released.

A singleton means that there's only one database connection object, to connect to the db. So if a second person wants to connect to it, they need to wait until they can access that object.

That's bad news.

Just keep creating new instances of the database connection object, when required. The trick here is to open the connection as late as possible and then close that connection as soon as possible.

The most expensive operation in a database connection object, is the actual connection. not the creation.

Pure.Krome
+2  A: 

No need for a Singleton. Here are some articles on connection pooling:

.NET 1.1

Connection Pooling for the .NET Framework Data Provider for SQL Server

.NET 2.0

Using Connection Pooling with SQL Server

.NET 3.0

Using Connection Pooling

.NET 3.5

SQL Server Connection Pooling (ADO.NET)

.NET 4.0

SQL Server Connection Pooling (ADO.NET)

rick schott