views:

341

answers:

5

Given the method:

internal static DataSet SelectDataSet(String commandText, DataBaseEnum dataBase)
{
    var dataset = new DataSet();

    SqlConnection sqlc = dataBase == DataBaseEnum.ZipCodeDb
                             ? new SqlConnection(ConfigurationManager.AppSettings["ZipcodeDB"])
                             : new SqlConnection(ConfigurationManager.AppSettings["WeatherDB"]);
    SqlCommand sqlcmd = sqlc.CreateCommand();
    sqlcmd.CommandText = commandText;
    var adapter = new SqlDataAdapter(sqlcmd.CommandText, sqlc);
    adapter.Fill(dataset);


    return dataset;
}

Why is sqlc (the SqlConnection) not disposed/close after the calling method goes out of scope or sqlc has no more references?

EDIT 1: Even wrapping it in using, I can still seeing the connection using (I have connection pooling turned off):

SELECT DB_NAME(dbid) as 'Database Name',
COUNT(dbid) as 'Total Connections'
FROM sys.sysprocesses WITH (nolock)
WHERE dbid > 0
GROUP BY dbid

EDIT 2: Have some more debugging with help I got from here - the answer was the someone hard coded a connection string with pooling. Thanks for all the help - if I could, I would mark all the responses as answers.

+14  A: 

C#'s garbage collection is non-deterministic but the language does provide a deterministic structure for resource disposal like this:

using (SqlConnection connection = new SqlConnection(...))
{
    // ...  
}

This will create a try/finally block which will ensure that the connection object is disposed regardless of what happens in the method. You really ought to wrap any instances of types that implement IDisposable in a using block like this as it will ensure responsibly resource management (of unmanaged resources like database connections) and also it gives you the deterministic control you are looking for.

Andrew Hare
+1 for neat answer, better than mine.
o.k.w
+1  A: 

It will be, after the garbage collection does it's job. Same goes for opening a file stream for writing without closing it. It might get 'locked' even though the codes went out of scope.

o.k.w
+2  A: 

Because c# is a garbage collected language, and garbage collection is not deterministic. The fact of it is that your sqlconnection is disposed. You just don't get to choose when.

Sql connections are a limited resource, and it's easily possible that you might create enough of them to run out. Write it like this instead:

internal static DataSet SelectDataSet(String commandText, DataBaseEnum dataBase)
{
    var dataset = new DataSet();

    using (SqlConnection sqlc = dataBase == DataBaseEnum.ZipCodeDb
                             ? new SqlConnection(ConfigurationManager.AppSettings["ZipcodeDB"])
                             : new SqlConnection(ConfigurationManager.AppSettings["WeatherDB"]))
    using (SqlCommand sqlcmd = sqlc.CreateCommand())
    {
        sqlcmd.CommandText = commandText;
        var adapter = new SqlDataAdapter(sqlcmd.CommandText, sqlc);
        adapter.Fill(dataset);

    }
    return dataset;
}

Though in this case you might get away with it, because the .Fill() method is a strange beast:

If the IDbConnection is closed before Fill is called, it is opened to retrieve data and then closed.

So that means the Data Adapter should be taking care of it for you, if you start out with a closed connection. I'm much more concerned that you're passing in your sql command as a plain string. There have to be user parameters in your queries from time to time, and this means you're concatenating that data directly into the command string. Don't do that!! Using the SqlCommand's Paramters collection instead.

Joel Coehoorn
+1  A: 

I believe it has something to do with SqlConnection pooling. What you can do, and we do often at work is wrap the entire call in a using statement which causes it to call the dispose() method, hense closing the connection and disposing of the object

You could then do something like this instead:


internal static DataSet SelectDataSet(String commandText, DataBaseEnum dataBase)
{
    var dataset = new DataSet();

    using(SqlConnection sqlc = dataBase == DataBaseEnum.ZipCodeDb
                             ? new SqlConnection(ConfigurationManager.AppSettings["ZipcodeDB"])
                             : new SqlConnection(ConfigurationManager.AppSettings["WeatherDB"])) {
        SqlCommand sqlcmd = sqlc.CreateCommand();
        sqlcmd.CommandText = commandText;
        var adapter = new SqlDataAdapter(sqlcmd.CommandText, sqlc);
        adapter.Fill(dataset);


        return dataset;
    }
}

Anthony Shaw
+1  A: 

I agree with all the answers here plus a connection may be a wider scope than just a method. When you need to use your existing connection in different places the scenario changes a bit. Always make sure to call Dispose for all objects that implement IDisposable after you are done using them. This is a good practice so you don't end up with unused objects that the garbage collector can't decide what to do with them.

O. Askari