Not allowed to change the 'ConnectionString' property. The connection's current state is open

Question

First time user of stackoverflow, but I have followed its development over on Coding Horror.

I am having a massive headache with the above error. I have ELMAH installed and Google Analytics and as the site traffic has increased, so has the number of times I have seen this error.

I have done my best to follow the Microsoft principles: http://msdn.microsoft.com/en-us/library/ms971481.aspx throughout development and I've optimised my code as much as possible based on multiple sources of advice across the web.

I have my SqlConnection in a public class;

Public Class pitstop
Public Shared oConn As New System.Data.SqlClient.SqlConnection
    Public Shared Sub doConnect()
    If oConn.State = ConnectionState.Closed Then
        oConn.ConnectionString = System.Configuration.ConfigurationManager.ConnectionStrings("pitstopConnectionString").ConnectionString
        oConn.Open()
    End If
End Sub
Public Shared Sub doGarbage()
    oConn.Dispose()
End Sub
' /// other code ///
End Class

And in my main application pages, I do much the same as this:

 Private Sub doPump()
    pitstop.doConnect()
    Dim cmd As New System.Data.SqlClient.SqlCommand("doGetCategory", pitstop.oConn)
    Dim dt As New DataTable
    Dim dr As SqlDataReader

    cmd.Parameters.Add("@cat", SqlDbType.Int)
    cmd.Parameters("@cat").Value = CType(Request.QueryString("id"), Integer)

    cmd.CommandType = CommandType.StoredProcedure

    dr = cmd.ExecuteReader()
    While dr.Read()
        If dr.HasRows = True Then
            litCategory.Text = dr("category")
            litCategoryDesc.Text = pitstop.doMakeReadyForHTML(dr("desc"))
        End If
    End While
    cmd = Nothing
    dr.Close()
    pitstop.doGarbage()
End Sub

I've used this method throughout and most of the time it works well, but now the site is getting horrifically busy, the dramas have begun! Does anyone have any ideas?

I would prefer not to have to re-write mountains of code, but I'm open to suggestions.

:)

Chris

Answer 1

Sharing your connections is the problem.

There is no need to share connections and creates problems like you are experiencing. .net's connection pooling handles the sharing of the real connections behind the scenes.

Just create a new connection in doPump()

Private Sub doPump()
    Using Dim conn As New SqlConnection(ConfigurationManager.ConnectionStrings("pitstopConnectionString").ConnectionString)
        Using Dim cmd As New SqlCommand("doGetCategory", conn)
        cmd.CommandType = CommandType.StoredProcedure
        cmd.Parameters.AddWithValue("@cat", CType(Request.QueryString("id"), Integer))
        conn.Open()
        Using Dim dr as SqlDataReader = cmd.ExecuteReader()
            While dr.Read()
                    litCategory.Text = dr("category")
                    litCategoryDesc.Text = pitstop.doMakeReadyForHTML(dr("desc"))
            End While
            dr.Close()
        End Using
    End Using
End Sub

Answer 2

There are so many problems with your code I don't actually know where to start.

First off, I assume that pitstop is actual class name, which is a horrible one. Second, pitstop.doMakeReadyForHTML makes me think that this class contains all sorts of functionality which does not really belong there.

Third, and most horrible one, is that your SqlConnection object is Shared. And that's in a web app, which is multithreaded by its very nature.

Fourth, your database access logic is in your code-behind page, which is bad.

Fifth, you do not manage your resources properly. What if exception occurs before call to pitstop.doGarbage() (awful name, by the way)? Connection will either never get closed, or it'll be hanging around and causing all sorts of bugs.

What I suggest is as follows. Catch up on design patterns and software architecture in general. Then you'll see all your problems yourself. Next, go read Patterns of Enterprise Application Architecture: this will give you a solid foundation to build up on.

As far as your current code goes: unShared your connection, move your data access logic to a separate class, manage resources properly (think Using statement).

Answer 3

I have added the following code having removed the 'shared' connection from the pitstop class. Pitstop is called 'pitstop' because that is the name of the project. Is it unsuitable? What conventions do you guys use?

I have scrapped the doGarbage() function and now call the connection and disposable 'inline', like so;

  Private Sub doPump()
    Dim oConn As New SqlConnection(System.Configuration.ConfigurationManager.ConnectionStrings("pitstopConnectionString").ConnectionString)
    Dim cmd As New System.Data.SqlClient.SqlCommand("doGetCategory", oConn)
    oConn.Open()

    Dim dt As New DataTable
    Dim dr As SqlDataReader

    cmd.Parameters.Add("@cat", SqlDbType.Int)
    cmd.Parameters("@cat").Value = CType(Request.QueryString("id"), Integer)

    cmd.CommandType = CommandType.StoredProcedure

    dr = cmd.ExecuteReader()
    While dr.Read()
        If dr.HasRows = True Then
            litCategory.Text = dr("category")
            litCategoryDesc.Text = pitstop.doMakeReadyForHTML(dr("desc"))
        End If
    End While
    cmd = Nothing
    dr.Close()

    oConn.Dispose()
End Sub

Is this correct?

In addition, Anton mentioned having functionality within the class as being incorrect - I will read the documents he has linked too - but is there are quick answer as to why it is wrong?

Many thanks to all the replies - you have all been very helpful.

Chris