Yahfoufi Yahfoufi -3 years ago 81
SQL Question

Managing and Closing Dynamically created SQL connections in .net

I have a c# windows form application that connects to databases dynamically where each user may connect to different databases.

The current implementation is as follows:

Connection Repository that contains a dynamically populated list of connections (per user).

When a user initiates a request that requires a database connection the respective connection is looked up from the connection repository ,opened , and then used in the user request .

Code Sample from the connection repository

public class RepoItem
{
public string databasename;
public SqlConnection sqlcnn;
}

public class ConnectionRepository
{
private List<RepoItem> connectionrepositroylist;

public SqlConnection getConnection(String dbname)
{
SqlConnection cnn = (from n in connectionrepositroylist
where n.databasename == dbname
select n.sqlcnn).Single;

cnn.Open();
return cnn;
}
}


sorry for any code errors i just improvised a small version of the implementation for demonstration purpose.

I'am not closing connections after a command execution because it may be used by another command simultaneously.

The questions are:

Should i be worried about closing the connections ?

Does connection close automatically if it is idle for a specific period ?

I have a method in mind to implement a timer in the created Connection Repository and check for idle connections through the
Executing
ConnectionState Enumeration
and close them manually.
Any suggestions are welcome .

When i want a specific connection i call the getConnection function in the ConnectionRepository class and pass the database name as a parameter.

PS: I didn't post the complete implemented code because it is quite big and includes the preferences that affect the populating of the connection list.

Answer Source

I would suggest not to return the SQLConnection to the calling method at all. Instead, create a method that will accept an Action<SqlConnection>, create the connection inside a using block, and execute the action inside that block

This way you know that the connection will always be correctly closed and disposed, while giving the using code the freedom to do whatever it needs:

public class RepoItem
{
    public string databasename;
    public SqlConnection sqlcnn;
}

public class DatabaseConnector
{
    private List<RepoItem> connectionrepositroylist;

    private SqlConnection GetConnection(String dbname)
    {
        return (from n in connectionrepositroylist
                where n.databasename == dbname
                select n.sqlcnn).SingleOrDefault();
    }

    public void Execute(String dbname, Action<SqlConnection> action)
    {
        using (var cnn = GetConnection(dbname))
        {
            if (cnn != null) // in case dbname is not in the list...
            {
                cnn.Open();
                action(cnn);
                cnn.Close();
            }
        }

    }
}

Then, to execute an sql statement you can do something like this:

public void ExecuteReaderExample(string dbName, string sql)
{
    Execute("dbName",
    connection =>
    {
        using (var cmd = new SqlCommand(sql, connection))
        {
            using (var reader = cmd.ExecuteReader())
            {
                while (reader.Read())
                {
                    // do stuff with data form the database
                }
            }
        }
    });
}

Of course, you can also wrap the SqlCommand in a method like this. I've been working with this approach for quite some time now, and as far as I can tell it's working well. In fact, It's working so well I've published a project on git hub based on this approach.
It saves you a lot of the plumbing when dealing with ado.net, by wrapping the connection, command, reader and adapter much the same way. Feel free to download it and adapt to your needs.

P.S. To answer your questions directly:

Should i be worried about closing the connections ?

Yes, you should.

Does connection close automatically if it is idle for a specific period ?

No, it doesn't.

However, implementing a method like I suggested will handle closing and disposing the connection object for you, so you don't need to worry about it.

Update

As Yahfoufi wrote in his comment, this design has a flaw, since multiple commands are using the same instance of SqlConnection, you are risking closing the connection while other commands are running. However, fixing this design flaw is very easy - instead of holding SqlConnection in RepoItem you can simply hold the connection string:

public class RepoItem
{
    public string DatabaseName {get; set;}
    public string ConnectionString {get; set;}
}

Then you change the GetConnection method like this:

    private SqlConnection GetConnection(String dbname)
    {
        return new SqlConnection(from n in connectionrepositroylist
                where n.databasename == dbname
                select n.sqlcnn).SingleOrDefault());
    }

Now each Execute method is working on it's own individual instance of SqlConnection so you don't need to worry about closing in the middle of some other command executing.

However, While we are on the subject of refactoring, I would suggest removing the RepoItem class all together and instead of using a List<RepoItem> to hold the connection strings simply use a Dictionary<string, string>, where the database name is the key and the connection string is the value. This way you can only have one connection string per database name, and your GetConnection method is simplified to this:

private Dictionary<string, string> connectionrepositroylist;

    private SqlConnection GetConnection(String dbname)
    {
        SqlConnection con = null;
        if(connectionrepositroylist.ContainsKey(dbname))
        {
            con = new SqlConnection(connectionrepositroylist[dbname]);
        }
        return con;
    }

So, the complete DatabaseConnector class will look like this:

public class DatabaseConnector
{
    private Dictionary<string, string> connectionrepositroylist;

    private SqlConnection GetConnection(String dbname)
    {
        SqlConnection con = null;
        if(connectionrepositroylist.ContainsKey(dbname))
        {
            con = new SqlConnection(connectionrepositroylist[dbname]);
        }
        return con;
    }

    public void Execute(String dbname, Action<SqlConnection> action)
    {
        using (var cnn = GetConnection(dbname))
        {
            if (cnn != null) // in case dbname is not in the list...
            {
                cnn.Open();
                action(cnn);
                cnn.Close();
            }
        }

    }

    // Of course, You will need a way to populate your dictionary - 
    // I suggest having a couple of methods like this to add, update and remove items.
    public bool AddOrUpdateDataBaseName(string dbname, string connectionString)
    {
        if(connectionrepositroylist.ContainsKey(dbname))
        {
            connectionrepositroylist[dbname] = connectionString;
        }
        else
        {
            connectionrepositroylist.Add(dbname, connectionString);
        }
    }
}
Recommended from our users: Dynamic Network Monitoring from WhatsUp Gold from IPSwitch. Free Download