How do I track down an Unclosed reader

Question

I've got a utility function in a much larger project that updates a backend SQL database. It's currently failing most times I use it, with the error:

There is already an open DataReader associated with this Command which must be closed first.

The code for the function is below:

Public Function Update_Data(what As String, Optional where As String = "",
                            Optional table As String = ThisAddIn.defaultTable) As Integer
    Try
        Dim cmd As New SqlCommand With {
            .Connection = conn
        }

        cmd.CommandText = "UPDATE " & table & " SET " & what

        If where <> "" Then
            cmd.CommandText &= " WHERE " & where
        End If

        Update_Data = cmd.ExecuteNonQuery
        cmd.Dispose()
    Catch ex As Exception
        Update_Data = 0
        Debug.WriteLine("SQL Error updating data:" & vbCrLf & ex.Message)
    End Try
End Function

I've gone through the rest of the code to make sure that whenever I have a SQLDataReader declared I later call reader.close(). I added the cmd.Dispose() line to this and all the other ExecuteNonQuery functions I could find - incase that helped?

Are there any other instances/types of reader that might not be being closed?

Instead of explicitly calling `cmd.Dispose()` it's better to wrap that code with a `using` statement. That way it automatically disposes of itself. It's possible that they aren't getting properly disposed of (outside of `cmd`, but you'll want that for the reader and the connection). — Dortimer, Jul 11 '19 at 14:42
I've changed `Dim cm as New SqlCommand` to `Using cmd As New SqlCommand` in both functions that use `ExecuteNonQuery` but that hasn't helped. Should this also be changed where I'm explicitly using and closing a reader? — Martin KS, Jul 11 '19 at 14:56
Yeah, you want the connection, command, and reader all implemented with a `using` statement. If there's an exception or any error they won't be properly disposed. Calling `Dispose()` explicitly is also unnecessary if you're implementing `using` statements. — Dortimer, Jul 11 '19 at 15:15
OK, just by messing with it until VisualStudio stops complaining it looks like the way to use `using` with a reader is to change `Dim reader As SqlDataReader reader = cmd.ExecuteReader` to `Using reader As SqlDataReader = cmd.ExecuteReader` and then remove `reader.close()`. Or is that now wrong? — Martin KS, Jul 11 '19 at 15:20
Yeah, you'll want to wrap that in a using statement as well to avoid the issues mentioned above. I don't know the exact syntax for it in vb.net though. — Dortimer, Jul 11 '19 at 15:35

score 0 · Answer 1 · answered Jul 11 '19 at 14:56

0

In the case of an Exception, you aren't disposing your command.

If you don't want to use Using, add a Finally

 Public Function Update_Data(what As String, Optional where As String = "",
                             Optional table As String = ThisAddIn.defaultTable) As Integer
    Dim cmd As SqlCommand
    Try
        cmd = New SqlCommand With {.Connection = conn}
        cmd.CommandText = "UPDATE " & table & " SET " & what
        If where <> "" Then
            cmd.CommandText &= " WHERE " & where
        End If
        Update_Data = cmd.ExecuteNonQuery
    Catch ex As Exception
        Update_Data = 0
        Debug.WriteLine("SQL Error updating data:" & vbCrLf & ex.Message)
    Finally
        cmd.Dispose()
    End Try
End Function

but Using might be simpler

 Public Function Update_Data(what As String, Optional where As String = "",
                             Optional table As String = ThisAddIn.defaultTable) As Integer
    Using cmd As New SqlCommand With {.Connection = conn}
        Try
            cmd.CommandText = "UPDATE " & table & " SET " & what
            If where <> "" Then
                cmd.CommandText &= " WHERE " & where
            End If
            Update_Data = cmd.ExecuteNonQuery
        Catch ex As Exception
            Update_Data = 0
            Debug.WriteLine("SQL Error updating data:" & vbCrLf & ex.Message)
        End Try
    End Using
End Function

answered Jul 11 '19 at 14:56

djv

15,168
7
48
72

The error was happening on the first instance of the function though - before any exception had happened. I put a breakpoint on the `ExecuteNonQuery`. Would an undisposed reader in another function (again due to an exception) cause the same issue? – Martin KS Jul 11 '19 at 15:00
@MartinKS Absolutely. You should always use `Using` or `Finally` on any IDisposable as long as the scope of the object can be limited to said `Using`, so that you are guaranteed to dispose it. I notice you also have a class-level `conn` object holding onto the connection. This should usually be recreated and disposed within the same scope. – djv Jul 11 '19 at 15:10
I have the class-level `conn` because making the connection was taking time and leaving it open sped things up. How do I tell if something is IDisposable so I don't keep making this mistake? – Martin KS Jul 11 '19 at 15:17
1

@MartinKS you can see more about managing your connection in [this question and its answers](https://stackoverflow.com/q/4439409/832052). Most things which maintain a connection to something, or reserve a hardware resource, or timers, are IDisposable. If you are unsure you can see if an object has a Dispose method. If it does you generally want to dispose it as soon as you are done with it. – djv Jul 11 '19 at 15:24
@MartinKS You can try typing `.Dispo` after it and if Intellisense suggests "Dispose()" for you then it is disposable :) – Andrew Morton Jul 11 '19 at 15:47
1

Thanks all - every second word of the module is now `using` and it seems to have solved the problem! – Martin KS Jul 11 '19 at 15:50

How do I track down an Unclosed reader

1 Answers1