views:

1213

answers:

3

Hello there!

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

+3  A: 

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
Chad Grant
+1 for beating me to it :-)
Jakob Christensen
+2  A: 

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).

Anton Gogolev
Thanks, Deviant and Anton. My code is a mess :( But I have been set in the right direction :) Thanks. I will review, change and post back here. Thanks again.
Chris Laythorpe
Chris: The third and fifth points are the one's most immediately pertinent to the question you're asking and the ones you should follow up on.
Derek Lawless
+1 for adding some helpful suggestions.
Derek Lawless
Dude ... I am sure you didn't start off writing perfect code. Calm your ego and try and help him without talking down to him
Chad Grant
If the OP has pressing problems of this nature, I don't think going away to swot up on Design Patterns and software architecture is going to be his first course of action. A bit more focused assistance is probably in order.
CJM
A: 

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

Chris Laythorpe
I updated my code with how you would want to do it. Sorry if there are minor syntax errors, I have not done VB in years. but that is how i'd do it in c#. Need to wrap your IDisposable objects (SqlConnection,SqlCommand etc) in Using Statements. http://msdn.microsoft.com/en-us/library/htd05whh(VS.80).aspx
Chad Grant
Thanks Deviant, stunning examples. You've been a great help. Many thanks.
Chris Laythorpe