Does this function release resources properly?

Question

I'm trying to make sure that I don't leave any loose ends open in my application and am concerned about a few but might get my answer from this one. I've "overriden" some functions so that way I can try and keep all the resources as clean and free as possible. So in this instance, I have a function called ExecuteReader which returns a DbDataReader as normal, but all I had to pass to it was a SQL string rather than recreating a DbCommand every time. I want to make sure that even though I'm unable to call dbCommand.Dispose() that it is actually doing so. Any and all help is appreciated.

Public Function ExecuteReader(ByVal strSQL As String) As DbDataReader
    Dim dbCommand = _dbConnection.CreateCommand()
    dbCommand.CommandText = strSQL
    dbCommand.Prepare()
    Return dbCommand.ExecuteReader()
End Function

I was thinking of using a using statement, but I remember seeing a thread where someone said they think it was causing them problems having a return in the using statement. Also, I'm not sure if this should be community wiki or not. If it should be, let me know. Thanks.

Code Update:

Here's an example of how I'm using it.

Public Sub RevertDatabase()
    'This function can be used whenever all changes need to be undone, but was created'
    'for saving the data as a .out file.  It sets all changes back to their original value.'

    'Set the data reader to all parts and columns that were changed.'
    _dbReader = ExecuteReader("SELECT PART_ID, PART_PREV_VALUE, REPORT_COLUMN_NAME FROM REPORTS WHERE PART_PREV_VALUE NOT NULL")
    'Create an instance of the Command class.'
    Dim cmd = New Command()

    While _dbReader.Read()
        'For each part and columns that has been changed, set the values in the'
        'new cmd variable and then update the value using the same function'
        'that is used whenever a value is changed in the data grid view.'
        cmd.CommandString = _dbReader("REPORT_COLUMN_NAME").ToString().Replace("_", " ")
        cmd.Value = _dbReader("PART_PREV_VALUE").ToString()
        cmd.ID = _dbReader("PART_ID").ToString()
        UpdateValue(cmd)
    End While

    'Close the reader.'
    _dbReader.Close()
End Sub

In here, I set the _dbReader to what I'd get from the function and eventually I close the _dbReader. I do not close the connection as I don't open it each time I make a query. This is a SQLite database that only one user will be using at a time (small application with very very very little likeliness it will grow) so I didn't think it necessary to close and open the connection all the time. Maybe I'm wrong, not sure. Using it this way though, it is potentially ok for cleaning resources?

Answer 1

It's a bad idea passing back DbDataReader as this requires the stream to be kept open and relies on calling code to do the right thing by disposing the reader. It's also makes it difficult to close the underlying command and connection objects. One way to facilitate this if you really want to expose the reader, is to use CommandBehavour. This can be used to close the underlying connection when the reader itself is closed.

dbCommand.ExecuteReader(CommandBehavour.CloseConnection)

As DbConnection, DbCommand and DbDataReader are Disposable you need to refactor the code to allow for these to be cleaned up when the code has finished with them. One way is to achieve this is to implement IDisposable within your own class and bubble dispose though to any encapsulated objects. The calling code can then implement using to ensure resources are freed.

Using helper As New DatabaseHelper()
   Using reader As IDataReader = helper.LoadSomeDataReader()

      ' do something with reader

   End Using
End Using

UPDATE:

Your second block of code would become more like:

Public Sub RevertDatabase()
   Using _dbReader As IDataReader = ExecuteReader(...)

      While _dbReader.Read()

         Using cmd As New Command()
            cmd.CommandString = _dbReader("REPORT_COLUMN_NAME").ToString().Replace("_", " ")
            cmd.Value = _dbReader("PART_PREV_VALUE").ToString()
            cmd.ID = _dbReader("PART_ID").ToString()
            UpdateValue(cmd)
         End Using

       End While

    End Using
 End Sub

You should always open and close the connection, rather then just leaving it open. It's could practice and although highly unlikely in a very small application, you could run into issues. You also shouldn't be keeping a reference to the _dbReader IMO.

Answer 2

Returning an IDisposable instance from a function is perfectly fine. The ownership of the instance is being transferred from the function to the caller and it is now the caller's responsibility to call Dispose. This is the exact same principal in play when calling IDbCommand.ExecuteReader.

You should definitely be using the Using statement. It makes the code more readable since it automatically inserts the correct try-finally block that calls the Dispose method. I do not know the details of the issue you were referring to, but you should not have any problems yourself since you are not doing anything out of the ordinary.

Public Sub RevertDatabase() 
    Using dbReader As DbDataReader = ExecuteReader(...) 
      // Use the data reader here.
    End Using
End Sub