ExecuteReader requires an open and available Connection. The connection’s current state is Connecting

Sorry for only commenting in the first place, but i’m posting almost every day a similar comment since many people think that it would be smart to encapsulate ADO.NET functionality into a DB-Class(me too 10 years ago). Mostly they decide to use static/shared objects since it seems to be faster than to create a new object for any action.

That is neither a good idea in terms of peformance nor in terms of fail-safety.

Don’t poach on the Connection-Pool’s territory

There’s a good reason why ADO.NET internally manages the underlying Connections to the DBMS in the ADO-NET Connection-Pool:

In practice, most applications use only one or a few different
configurations for connections. This means that during application
execution, many identical connections will be repeatedly opened and
closed. To minimize the cost of opening connections, ADO.NET uses an
optimization technique called connection pooling.

Connection pooling reduces the number of times that new connections
must be opened. The pooler maintains ownership of the physical
connection. It manages connections by keeping alive a set of active
connections for each given connection configuration. Whenever a user
calls Open on a connection, the pooler looks for an available
connection in the pool. If a pooled connection is available, it
returns it to the caller instead of opening a new connection. When the
application calls Close on the connection, the pooler returns it to
the pooled set of active connections instead of closing it. Once the
connection is returned to the pool, it is ready to be reused on the
next Open call.

So obviously there’s no reason to avoid creating,opening or closing connections since actually they aren’t created,opened and closed at all. This is “only” a flag for the connection pool to know when a connection can be reused or not. But it’s a very important flag, because if a connection is “in use”(the connection pool assumes), a new physical connection must be openend to the DBMS what is very expensive.

So you’re gaining no performance improvement but the opposite. If the maximum pool size specified (100 is the default) is reached, you would even get exceptions(too many open connections …). So this will not only impact the performance tremendously but also be a source for nasty errors and (without using Transactions) a data-dumping-area.

If you’re even using static connections you’re creating a lock for every thread trying to access this object. ASP.NET is a multithreading environment by nature. So theres a great chance for these locks which causes performance issues at best. Actually sooner or later you’ll get many different exceptions(like your ExecuteReader requires an open and available Connection).

Conclusion:

• Don’t reuse connections or any ADO.NET objects at all.
• Don’t make them static/shared(in VB.NET)
• Always create, open(in case of Connections), use, close and dispose them where you need them(f.e. in a method)
• use the using-statement to dispose and close(in case of Connections) implicitely

That’s true not only for Connections(although most noticable). Every object implementing IDisposable should be disposed(simplest by using-statement), all the more in the System.Data.SqlClient namespace.

All the above speaks against a custom DB-Class which encapsulates and reuse all objects. That’s the reason why i commented to trash it. That’s only a problem source.

---

Edit: Here’s a possible implementation of your retrievePromotion-method:

public Promotion retrievePromotion(int promotionID)
{
    Promotion promo = null;
    var connectionString = System.Configuration.ConfigurationManager.ConnectionStrings["MainConnStr"].ConnectionString;
    using (SqlConnection connection = new SqlConnection(connectionString))
    {
        var queryString = "SELECT PromotionID, PromotionTitle, PromotionURL FROM Promotion WHERE PromotionID=@PromotionID";
        using (var da = new SqlDataAdapter(queryString, connection))
        {
            // you could also use a SqlDataReader instead
            // note that a DataTable does not need to be disposed since it does not implement IDisposable
            var tblPromotion = new DataTable();
            // avoid SQL-Injection
            da.SelectCommand.Parameters.Add("@PromotionID", SqlDbType.Int);
            da.SelectCommand.Parameters["@PromotionID"].Value = promotionID;
            try
            {
                connection.Open(); // not necessarily needed in this case because DataAdapter.Fill does it otherwise 
                da.Fill(tblPromotion);
                if (tblPromotion.Rows.Count != 0)
                {
                    var promoRow = tblPromotion.Rows[0];
                    promo = new Promotion()
                    {
                        promotionID    = promotionID,
                        promotionTitle = promoRow.Field<String>("PromotionTitle"),
                        promotionUrl   = promoRow.Field<String>("PromotionURL")
                    };
                }
            }
            catch (Exception ex)
            {
                // log this exception or throw it up the StackTrace
                // we do not need a finally-block to close the connection since it will be closed implicitely in an using-statement
                throw;
            }
        }
    }
    return promo;
}