SQL Server connections in ASP.NET application

Question

I have created a web application with asp.net, roughly 100 users are using it.

However from time to time people are getting the error message that the connection is still open. Indicating that it was not closed properly.

It appears on random places, not one specific place and no other errors before it.

I know that when I have a bug in the application and it crashes without me gracefully dealing with the error the connection remains open as well and basically everyone will crash because of it. This made me think that everyone uses the same connection object, is it possible that 2 users might have the perfect timing and invoke a function using a DB connection at the same time causing the error? Is there a way to make sure everyone uses their own connection objects, like put it in their session or something?

I hope you can understand what I mean, I don't think posting any of my code will help since it happens on random places within my project.

They are connection to a SQL Server using System.Data.SqlClient.

Find below the function which generates the error. This function is called by the Page_Load, nothing is before it.

public static SqlConnection conn = new SqlConnection("Data Source=Server00\\SQLEXPRESS;Initial Catalog=r2;Integrated Security=true;Connect Timeout=0");

private void populateGameDrop()
{
    try
    {
        conn.Open();
        drop_game.Items.Clear();

        SqlCommand cmd = conn.CreateCommand();
        Access ac = (Access)Session["Access"];
        cmd.CommandText = "Select * from dbo.Games where " + ac.GameQuery;

        SqlDataReader r = cmd.ExecuteReader();

        while (r.Read())
        {
            drop_game.Items.Add(new ListItem(r["name"].ToString(), r["Abbr"].ToString()));
        }

        conn.Close();
    }
    catch (Exception exc)
    {
        conn.Close();
        Log.Error(exc.ToString());
        Session["Error"] = exc.ToString();
        Response.Redirect("~/YouBrokeIt.aspx");
    }

    populateServers();
    SetSplitScreen();
}

I _think_ it is a good practice to open and close the connection on a per request basis. This [answer](https://stackoverflow.com/questions/10585478/one-dbcontext-per-web-request-why/10588594#10588594) is based on Entity Framework's `DbContext` but I think it will apply to plain ADO, too. — Alex Booker, Jul 31 '15 at 07:12
Sorry, but without being able to **pinpoint** what code snippet causes these problems, this question is really waaay too broad. You'll need to do some more research on your own first, to be able to tell us *what* code is causing the issues — marc_s, Jul 31 '15 at 07:13
Add some code of how you are accessing the `DB` are you using `using` staments? — 3dd, Jul 31 '15 at 07:14
Basic suggestion is: create `SqlConnection` objects as late as possible - preferable immediately before creating an `SqlCommand` object that uses it, and wrap both of them in `using` statements. The only thing you should aim to be sharing around is the connection *string*. — Damien_The_Unbeliever, Jul 31 '15 at 07:16
`static SqlConnection` shouldn't be static, it should be created inside a using stement — 3dd, Jul 31 '15 at 07:24
See @Damien_The_Unbeliever's comment - **don't** use `static` connections, create them as late as possible in a `using() { ... }` block. Set up command locally, too - in a `using() { ... }` block as well. Open connection, execute your command (or iterate over your reader), close connection as fast as possible — marc_s, Jul 31 '15 at 07:24

Damien_The_Unbeliever · Accepted Answer · 2015-07-31T07:33:48.993

Don't try to share SqlConnection objects.

Try this instead:

private static string connString = "Data Source=Server00\\SQLEXPRESS;Initial Catalog=r2;Integrated Security=true;Connect Timeout=0";
private void populateGameDrop()
{
    try
    {
        using (var conn = new SqlConnection(connString))
        {
            conn.Open();
            drop_game.Items.Clear();
            using (var cmd = conn.CreateCommand())
            {
                Access ac = (Access)Session["Access"];
                //TODO
                //TODO - Introduce parameters to avoid SQL Injection risk
                //TODO
                cmd.CommandText = "Select name,Abbr from dbo.Games where " + ac.GameQuery;
                using(SqlDataReader r = cmd.ExecuteReader())
                {
                    while (r.Read())
                    {
                        drop_game.Items.Add(new ListItem(r["name"].ToString(),
                                            r["Abbr"].ToString()));
                    }
                }
            }
        }
    }
    catch (Exception exc)
    {
        Log.Error(exc.ToString());
        Session["Error"] = exc.ToString();
        Response.Redirect("~/YouBrokeIt.aspx");
    }
    populateServers();
    SetSplitScreen();
}

Behind the scenes, .NET uses a concept called connection pooling so that the actual number of real connections to SQL Server are minimized. But SqlConnection objects aren't designed to be shared by multiple threads.

You should *also* put your `SqlDataReader` into a `using() { .... }` block (if you're already doing it for `SqlConnection` and `SqlCommand` ....) - and you should also **parametrize** the query to avoid SQL injection — marc_s, Jul 31 '15 at 07:30
I see :O This is very enlightening really.. Looks like ill be doing a lot of editting to my project. as for the parameter suggestion, have been doing it on other queries, but this is bassicly a static query. No user input. Just the where is variable on earlier done functions. — Kage, Jul 31 '15 at 07:32

score 1 · Answer 2 · answered Jul 31 '15 at 07:58

Don't place your database code directly in your ASPX pages. Creating an extra layer (i.e. DAL) allows you to test the DB methods without using the page.

Try something like this.

//Don't embed database logic directly in the aspx files
public class GamesProvider
{        
    //Put the ConnectionString in you configuration file
    private string ConnectionString
    {
        get { return ConfigurationManager.ConnectionStrings["GameDB"].ConnectionString; }
    }

    public IEnumerable<Game> LoadGames(string x, string y)
    {
        var games = new List<Game>();

        const string queryString = "select name, Abbr from dbo.Games where x = @x and y = @y";

        using (var connection = new SqlConnection(ConnectionString))
        using (var command = new SqlCommand(queryString, connection))
        {
            command.Parameters.AddWithValue("@x", x);
            command.Parameters.AddWithValue("@y", y);
            using (var dateReader = command.ExecuteReader())
            {
                while (dateReader.Read())
                {
                    var game = new Game
                    {
                        Name = dateReader["name"].ToString(),
                        Abbr = dateReader["Abbr"].ToString(),
                    };
                    games.Add(game);
                }
            }
        }

        return games;
    }
}

//Use types
public class Game
{
    public string Name { get; set; }
    public string Abbr { get; set; }
}

score 0 · Answer 3 · edited May 23 '17 at 12:29

0

Your SQL connections shouldn't be static, use the following to create them

var connectionString = "YOUR CONNECTION STRING";
var queryString = "SQL QUERY";

using (SqlConnection connection = new SqlConnection(connectionString))
using (SqlCommand command = new SqlCommand(queryString, connection))
using (SqlDataReader dateReader = command.ExecuteReader()) {

}

VERY IMPORTANT

You should use parameterised SQL your code is open to SQL injection attacks.

please see Parameterize SQL query

edited May 23 '17 at 12:29

Community

1
1

answered Jul 31 '15 at 07:25

3dd

2,520
13
20

You should really use `using() { .... }` blocks for `SqlConnection`, `SqlCommand` and `SqlDataReader` – marc_s Jul 31 '15 at 07:31
@marc_s good point I've updated my answer – 3dd Jul 31 '15 at 07:36

SQL Server connections in ASP.NET application

3 Answers3