SqlDataAdapter Close connection method

Question

I have a code like this in my program and I believe that it's not closing the connection after that the data is getting filled in.

public static string ConnectionInfo = System.Configuration.ConfigurationManager.ConnectionStrings["Default"].ConnectionString;
public static DataTable ExecuteQuery(string query, string table)
    {
        SqlConnection cnn = new SqlConnection(ConnectionInfo);
        SqlDataAdapter Adp = new SqlDataAdapter(query, cnn);
        DataSet Ds = new DataSet();
        Adp.Fill(Ds, table);
        return Ds.Tables[table];
    }

Is there any problem in this code ?

Do you actually close the connection somewhere? – David Pilkington Nov 20 '13 at 09:08 — David Pilkington, Nov 20 '13 at 09:08
`cnn.Close()` or `using`............? – Arran Nov 20 '13 at 09:10 — Arran, Nov 20 '13 at 09:10

Tim Schmelter · Accepted Answer · 2013-11-20T09:32:24.613

4

The only problem is that you are not using the using statement for the SqlConnection and the DataAdapter. However, DbDataAdapter.Fill opens and closes the connection implicitely.

public static DataTable ExecuteQuery(string query, string table)
{
    using(SqlConnection cnn = new SqlConnection(ConnectionInfo))
    using(SqlDataAdapter Adp = new SqlDataAdapter(query, cnn))
    {
        DataTable tbl = new DataTable();
        Adp.Fill(tbl);
        return tbl;
    }
}

The connection object associated with the SELECT statement must be valid, but it does not need to be open. If the connection is closed before Fill is called, it is opened to retrieve data, then closed. If the connection is open before Fill is called, it remains open.

Note that

the using statement will close the connection implicitely even on error
i have used DataAdapter.Fill(DataTable) because you're using a single table anyway

Edit: i've only just noticed that you are using a parameter for the table-name. You can also use DbDataAdapter.Fill(DataSet, String) instead. That does not change anything.

edited Nov 20 '13 at 09:32

answered Nov 20 '13 at 09:10

Tim Schmelter

450,073
74
686
939

I think that the second `using` statement must be included into the first one's brackets. – Alberto Solano Nov 20 '13 at 09:14
1

@AlbertoSolano: No, the second using is treated as a single statement because of the braces, therefore it is part of the first using(similar to this `if`: `if (true) if (true) {;}`). – Tim Schmelter Nov 20 '13 at 09:15
Ah I understood, thanks. I never tried this approach. I always used brackets even for one single line, for better readability. :-) – Alberto Solano Nov 20 '13 at 09:19
@AlbertoSolano: It's a matter of taste, but i prefer this approach since it prevents indentation. Consider that there are even more disposable objects involved like a `SqlCommand`, then you have already three intendations and the relevant code will scroll out of sight. The `SqlConnection` initialization is not interesting. Thats why i always omit the braces. – Tim Schmelter Nov 20 '13 at 09:23

score 0 · Answer 2 · answered Nov 20 '13 at 09:09

Add a using statement in order to close the connection reliably. This ensures that the connection is closed even if an exception occurs. Change your code as follows:

public static DataTable ExecuteQuery(string query, string table)
    {
        using(SqlConnection cnn = new SqlConnection(ConnectionInfo))
        {
            SqlDataAdapter Adp = new SqlDataAdapter(query, cnn);
            DataSet Ds = new DataSet();
            Adp.Fill(Ds, table);
            return Ds.Tables[table];
        }
    }

score -1 · Answer 3 · answered Nov 20 '13 at 09:33

Whatever the opening/closing of the connections should be done in try-catch-finally block.

And we should not be using "using" [using (SqlConnection connection = new SqlConnection(connectionString))] block. Because if something goes wrong with the network or any exception cause. Connection is not closed. So better to be use try-catch block.

    public static DataTable ExecuteQuery(string query, string table)
    {
        DataSet Ds = new DataSet();

        SqlConnection cnn = new SqlConnection(ConnectionInfo);

        try{
            SqlDataAdapter Adp = new SqlDataAdapter(query, cnn);
            Adp.Fill(Ds, table);
            return Ds.Tables[table]; 
        }
        catch{
            throw;
        }
        finally{
            cnn.Close();
        }

    }

_"we should not use the using block because if something goes wrong with the network or any exception cause the connection is not closed"_ This is simply wrong, `SqlConnection.Dispose` will call `Close` implicitly, even on error. So actually a simple `using` statement will be translated to your `try/finally`. — Tim Schmelter, Nov 20 '13 at 09:39

SqlDataAdapter Close connection method

3 Answers3