Wrapper that properly disposes and closes database objects

Question

I have a simple helper function that looks up data in a SQL Server database and returns a dataset.

This function is used in lot of different places in my web application.

For the most part, it works great. However when there are a lot of concurrent users all connected at the same time, I will occasionally get errors like this:

DBUtilities.getDataSet: A network-related or instance-specific error occurred while establishing a connection to SQL Server. The server was not found or was not accessible. Verify that the instance name is correct and that SQL Server is configured to allow remote connections.

When this happens, I don't see any errors in the database server event log, only the web server event logs.

After reading about garbage collection and disposing of objects, I'm starting to think that maybe the database connections are not getting closed or disposed of properly.

I was wondering, would there be a way to place this code in some type of wrapper that could help dispose of the database objects properly?

Here is the function I am referring too:

Public Overloads Function getDataSet(ByVal commandText As String, Optional ByVal tableName As String = "") As DataSet

    Dim ds As New DataSet
    Dim conn As New SqlConnection(myConnStr)

    Try
        Dim cmd As New SqlCommand(commandText, conn)
        cmd.CommandTimeout = 30
        cmd.CommandType = CommandType.Text
        Dim da As New SqlDataAdapter
        da.SelectCommand = cmd
        conn.Open()

        If String.IsNullOrEmpty(tableName) Then
            da.Fill(ds)
        Else
            da.Fill(ds, tableName)
        End If
    Catch ex As Exception
        Throw New Exception("AppDataMethods.getDataSet: " & ex.Message & ", cmdText = " & commandText)
    Finally
        conn.Close()
        conn = Nothing
    End Try
    Return ds

End Function

Joel Coehoorn · Accepted Answer · 2018-09-25T17:23:22.043

The Close() call is in a Finally block, so you're okay. Modern code tends to prefer Using blocks instead, and the conn = Nothing line is not helpful at all in VB.Net (it was needed in VB6/VBScript, but no more), but what you have should be okay. You also don't need to call conn.Open() if you're using a DataApater's Fill() method.

The scariest thing to me is asking for the commandText string, with no way to accept separate parameter data. This practically forces you to build badly insecure SQL strings in other code, or still encourages it as the default way to handle SQL even if the Overloads declaration accounts for this elsewhere.

My own data methods tend to always require passing an SqlParameter array, even for those queries with no parameters (use an empty array), to ensure no other programmers can use my database without having at least some awareness of query parameters. Alternatively, I sometimes use a functional approach, and instead have a delegate/lambda argument for assigning parameters.

I would also tend to build this method as Private, in a new Module dedicated to database access, and then make public methods there for each of your existing queries. If you have many queries, this could also be a Friend method in a separate class library project, so you have several classes or modules to organize queries into logical groups.

Private Overloads Function getDataSet(ByVal commandText As String, parameters As SqlParameter(), Optional ByVal tableName As String = "") As DataSet
    Dim result As New DataSet
    Using conn As New  SqlConnection(myConnStr), _
          cmd As New SqlCommand(commandText, conn), _
          da As New SqlDataAdapter(cmd)

        cmd.CommandTimeout = 30
        If parameters IsNot Nothing AndAlso parameters.Length > 0 Then
            cmd.Parameters.AddRange(parameters)
        End If

        If String.IsNullOrWhitespace(tableName) Then tableName = "Table1"
        da.Fill(result, tableName)

    End Using
    Return result    
End Function

I also removed the Catch block, since the only other thing it did was re-throw a similar exception to what you already have. You lose the CommandText, but if you have dedicated methods for each query and use parameterized queries, as recommended elsewhere, it's rare to have much info in the CommandText you wouldn't get directly from the stack trace.

The OP's code didn't call `conn.Dispose()` - could that have been the problem? — Andrew Morton, Sep 25 '18 at 16:59
Thanks, this code looks so much cleaner. Do you think this new code would improve performance at all? — SkyeBoniwell, Sep 25 '18 at 17:00
@AndrewMorton Doubtful, as it did call .Close(). But it's possible there's some interaction, but Close() should be all you need. — Joel Coehoorn, Sep 25 '18 at 17:00
@SkyeBoniwell If it means you're using parameters when otherwise you weren't, then yes, as parameterized queries will help you save a query compilation step on the database server. Also yes in cases of an exception, because we save a stack unroll and re-throw. Otherwise it's basically simpler ways to express the same thing. — Joel Coehoorn, Sep 25 '18 at 17:01
@SkyeBoniwell If you're having SQL Server performance problems, have a look at [How to Disable Auto Close in SQL Server](https://www.brentozar.com/blitz/auto-close-enabled/), make sure there are useful indexes in the database, and for the SQL parameters you're going to be using, make sure to specify the database column size for string-type parameters (nvarchar etc.). — Andrew Morton, Sep 25 '18 at 17:31
@Andrew Morton About `.Close()` / `.Dispose()` of the `OleDbConnection`, it's interesting to read the notes left on the .Net source of [DbConnectionInternal](https://referencesource.microsoft.com/#System.Data/System/Data/ProviderBase/DbConnectionInternal.cs,378). You could infer from the reading, that a `using` block here is more than "good practice". It could be considered mandatory. — Jimi, Sep 25 '18 at 23:41

Wrapper that properly disposes and closes database objects

1 Answers1