Search code examples
vb.netidisposabledataadapter

DataAdapter is disposed before reaching "End Using"


I know that I should always dispose DataAdapter instances. In most cases I'm disposing it immediately after closing the connection, but in cases like when user will be modifying DataTable items (displayed in ListBox or DataGridView) I create the DataAdapter, use it to fill the DataTable, but don't dispose it until the user clickes Save which calls DataAdapter.Update(DataTable)... not my main question but is this the right approach?

Back to the main question, I have these two functions:

Public Function LoadCompaniesDT(ByRef dtCompanies As DataTable) As Boolean
    Using daCompanies As MySqlDataAdapter = Nothing
        Return LoadCompaniesDT(daCompanies, dtCompanies)
    End Using
End Function

Public Function LoadCompaniesDT(ByRef daCompanies As MySqlDataAdapter, ByRef dtCompanies As DataTable) As Boolean
    Dim sql As String = "SELECT * FROM companies"
    Return LoadDT(daCompanies, dtCompanies, sql, Res.CompaniesFailedMsgBody)
End Function

They're used to call LoadDT which fills the DataTable so I have the choice to pass a DataAdapter or not.

Now I'm confused about something: When using the first LoadCompaniesDT function, daCompanies is disposed before reaching End Using.. like this:

Public Function LoadCompaniesDT(ByRef dtCompanies As DataTable) As Boolean
    Using daCompanies As MySqlDataAdapter = Nothing
        Dim tmp As Boolean = LoadCompaniesDT(daCompanies, dtCompanies)
        Console.WriteLine(daCompanies Is Nothing) ' ==> True!!
        Return tmp
    End Using
End Function

Note: if I use Dim daCompanies instead of Using daCompanies then daCompanies Is Nothing will return False.


LoadDT function code:

Private Function LoadDT(ByRef da As MySqlDataAdapter, ByRef dt As DataTable,
                                                      ByVal sqlQuery As String,
                                                      ByVal errorText As String) As Boolean
    Dim connStr As String = String.Format("server={0}; port={1}; user id={2}; password={3}; database={4}",
                                          DbServer, DbServerPort, DbUserName, DbPassword, DatabaseName)
    Dim conn As MySqlConnection = New MySqlConnection(connStr)
    Dim cmd As MySqlCommand = New MySqlCommand

    Try
        conn.Open()

        cmd.CommandType = CommandType.Text
        cmd.CommandText = sqlQuery
        cmd.Connection = conn

        da = New MySqlDataAdapter(cmd)
        dt = New DataTable
        da.Fill(dt)

        Return True
    Catch ex As Exception
        MessageBox.Show(errorText, Res.ServerError, MessageBoxButtons.OK, MessageBoxIcon.Error)
        Return False
    Finally
        cmd.Dispose()
        cmd = Nothing
        conn.Close()
        conn.Dispose()
    End Try
End Function

Solution

  • Update: you're right, you don't get an initialized MySqlDataAdapter back from the methods if the ByRef passed instance is used in a Using-statement. Those variables are readonly. In C# you get this meaningful compiler error:

    Error CS1657 Cannot pass 'daCompanies' as a ref or out argument because it is a 'using variable'

    It's documented here:

    Compiler Error CS1657

    Cannot pass 'parameter' as a ref or out argument because 'reason'' This error occurs when a variable is passed as a ref or out argument in a context in which that variable is readonly. Readonly contexts include foreach iteration variables, using variables, and fixed variables.

    In VB.NET you can do that(so the compiler ignores it which is almost a bug) but the variable is not initialized afterwards. But as mentioned below, you should not use this approach anyway.


    According to the the other question:

    If you look at the sample on MSDN you'll see that microsoft also doesn't dispose the dataadapter. So it not really necessary. Having said that, it's always best practise to use the Using statement on anything that implements IDisposable.

    A DataAdapter is not an expensive object and it does not hold unmanaged resources(like the connection). So it doesn't hurt to create a new instance from it whereever you need one. And you don't need to dispose it, but that's an implementation detail that might change in future or in a different implementation of DbDataAdapter, so it's still best practise to dispose it, best by using the Using-statement.

    I wouldn't use your approach because you are passing the sql-string to the method which often leads to a sql injection vulnerabiliy. Instead use sql parameters.

    For example:

    Private Function LoadDT() As DataTable
        Dim tbl As New DataTable()
        'Load connection string from app.config or web.config
        Dim sql As String = "SELECT * FROM companies" ' don't use * but list all columns explicitely
        Using conn As New MySqlConnection(My.Settings.MySqlConnection)
            Using da = New MySqlDataAdapter(sql, conn)
                da.Fill(tbl)
            End Using
        End Using
        Return tbl
    End Function
    

    Instead of passing an errorText ByRef i'd use a logging framework like log4net.