0

Please have a look at the code below:

Public Class Form1
    Private _ConString As String
    Private Sub Form1_Load(ByVal sender As Object, ByVal e As System.EventArgs) Handles Me.Load
        Dim objDR As SqlDataReader
        Dim objCommand As SqlCommand
        Dim objCon As SqlConnection
        Dim id As Integer
        Try
            _ConString = ConfigurationManager.ConnectionStrings("TestConnection").ToString
            objCon = New SqlConnection(_ConString)
            objCommand = New SqlCommand("SELECT * FROM Person")
            objCommand.Connection = objCon
            objCon.Open()
            objDR = objCommand.ExecuteReader(ConnectionState.Closed)
            Do While objDR.Read
                id = objDR("URN")
            Loop
            objDR.Close() 'line 16
        Catch ex As Exception
            throw
        Finally

        End Try

    End Sub
End Class

The connection object is closed on line 16. Please have a look at the code below:

Private Sub Form1_Load(ByVal sender As Object, ByVal e As System.EventArgs) Handles Me.Load
        Dim objDR As SqlDataReader
        Dim objCommand As SqlCommand
        Dim objCon As SqlConnection
        Dim id As Integer
        Try
            _ConString = ConfigurationManager.ConnectionStrings("TestConnection").ToString
            objCon = New SqlConnection(_ConString)
            objCommand = New SqlCommand("SELECT * FROM Person")
            objCommand.Connection = objCon
            objCon.Open()
            Using objCon
                Using objCommand
                    objDR = objCommand.ExecuteReader()
                    Do While objDR.Read
                        id = objDR("URN")
                    Loop
                End Using
            End Using
            objDR.Close() 'line 16
        Catch ex As Exception
           throw
        Finally

        End Try

    End Sub

I notice in both cases that the connection object and command object both still have state after they are closed (either by closing the data reader as per code sample 1 or moving outside the Using statement as per code sample 2). Could this be a source of a memory leak?

w0051977
  • 15,099
  • 32
  • 152
  • 329
  • Use `Using` also for your `SqlDataReader` since it 1. also implements `IDisposable` and 2. holds a reference to your connection. – Tim Schmelter Nov 21 '12 at 20:04
  • @Tim Schmelter, thanks. I am trying to implement a data tier like this: http://www.sharpdeveloper.net/source/SqlHelper-Source-Code-cs.html. Therefore I have to implement option 2. Please would you post your response in an answer? – w0051977 Nov 21 '12 at 20:06
  • You are missing the point of that link. That is the MS Enterprise library (and a really old version), which you can use without having to recreate from scratch. – StingyJack Nov 21 '12 at 20:20
  • @StingyJack, thanks. The reason I have to rewrite some of it is because I8 connect to an Oracle database and an SQL database. The database I connect to is chosen at runtime. – w0051977 Nov 21 '12 at 21:13
  • EL does that at runtime. http://stackoverflow.com/q/326365/16391 – StingyJack Nov 26 '12 at 17:24

2 Answers2

0

You should be doing something like this...

Using Conn as new SqlConnection(_ConString)
 Dim cmd as New SqlCommand(Conn, "Select * FROM Person");
   id = Convert.ToInt32(cmd.ExecuteScalar())
End Using

Wrap this in a try if you must, but only if you are going to handle it in some way. (A simple "Throw" is not handling anything.)

You are also using a DataReader (iterative, forward only) data access component to get a single value. You may want to use an ExecuteScalar() to simplify that code.

Aside from that, do not confuse "Disposed" as being the same as "Closed". The .NET framework manages garbage collection for you. If you want to dispose the connection and command objects, call .Dispose() on them (but the Using will take care of this for you!!!)

StingyJack
  • 19,041
  • 10
  • 63
  • 122
0

re: Could this be a source of a memory leak? no, it shouldn't be.

also, this:

            objDR = objCommand.ExecuteReader()
            Do While objDR.Read
                id = objDR("URN")
            Loop

looks bad because it iterates through all the rows and overwriting the id values. The last row set ends up with the value in ID. You're taking one result from a set of more than one rows. If you just want the max(URN) from Person, you can write your command to return that result.

Beth
  • 9,531
  • 1
  • 24
  • 43