Connection is not closed properly ASP.NET C#

Question

I have this button click event. Been trying to replace the con.Close() in different lines of code, tried for hours but couldn't fix. Maybe a second pair of eyes can help?

Error: System.InvalidOperationException: 'The connection was not closed. The connection's current state is open.'

protected void Button1_Click(object sender, EventArgs e)
{
    SqlConnection con = new SqlConnection();
    con.ConnectionString = ConfigurationManager.ConnectionStrings["ConnStr"].ConnectionString;
    con.Open();

    string query = "SELECT CATEGORY FROM CATEGORY WHERE C_UserName = '" + Session["id"] + "'  AND  CATEGORY = '" + DropDownList1.SelectedItem.Value + "' ";
    SqlCommand cmd = new SqlCommand(query, con);

    SqlDataReader reader = cmd.ExecuteReader();
    if (reader.HasRows)
    {
        cmd.Parameters.AddWithValue("@CATEGORY", DropDownList1.SelectedItem.Value);

        lblResult.Text = "You have selected this category. Please select a new category";
        con.Close();
    }
    else
    {
        SqlCommand cmd1 = new SqlCommand("UPDATE SET CATEGORY CCID@CCID (CATEGORY, C_USERNAME, CCID) VALUES (@CATEGORY, @C_USERNAME, @CCID)", con);
        cmd1.Parameters.AddWithValue("CATEGORY", DropDownList1.SelectedItem.Value);
        cmd1.Parameters.AddWithValue("C_USERNAME", Session["id"]);
        cmd1.Parameters.AddWithValue("CCID", Label1.Text);

        con.Open();
        int i = cmd1.ExecuteNonQuery();
        con.Close();

        if (i != 0)
        {
            Label2.Text = " Your data is been saved in the database";
            Label2.ForeColor = System.Drawing.Color.ForestGreen;
        }
        else
        {
            Label2.Text = "Something went wrong with selection";
            Label2.ForeColor = System.Drawing.Color.Red;
        }
    }
}

You should use "using" or closing connection outside any if. — Leandro Bardelli, Feb 03 '21 at 12:40
Remove `con.Open();` inside the `else`. It is not needed. You opened the door once, then tried to open it again and the door is saying "sorry, I am already open". — mjwills, Feb 03 '21 at 12:42
`using var con = new SqlConnection();` makes sure it always disposes it, even in case of an exception. Close is then no longer necessary, because it happens on disposal, unless you want to close earlier then exiting the code block. — Silvermind, Feb 03 '21 at 12:43
`UPDATE SET CATEGORY CCID@CCID (CATEGORY, C_USERNAME, CCID) VALUES (@CATEGORY, @C_USERNAME, @CCID)` does not look like valid SQL. — mjwills, Feb 03 '21 at 12:45
`"SELECT CATEGORY FROM CATEGORY WHERE C_UserName = '" + Session["id"] + "' AND CATEGORY = '" + DropDownList1.SelectedItem.Value + "' "; SqlCommand cmd = new SqlCommand(query, con);` **Do not do that**. https://stackoverflow.com/questions/14376473/what-are-good-ways-to-prevent-sql-injection — mjwills, Feb 03 '21 at 12:46

Athanasios Kataras · Answer 1 · 2021-02-03T14:11:08.793

Try this (open connection only once and close only once):

protected void Button1_Click(object sender, EventArgs e) {
  using(SqlConnection con = new SqlConnection()) {
    con.ConnectionString = ConfigurationManager.ConnectionStrings["ConnStr"].ConnectionString;

    string query = "SELECT CATEGORY FROM CATEGORY WHERE C_UserName = '" + Session["id"] + "'  AND  CATEGORY = '" + DropDownList1.SelectedItem.Value + "' ";
    SqlCommand cmd = new SqlCommand(query, con);
    con.Open();
    SqlDataReader reader = cmd.ExecuteReader();
    bool hasRows = reader.HasRows;
    reader.Close();
    if (hasRows) {
      // This line makes no sense after the execution of the query.
      //cmd.Parameters.AddWithValue("@CATEGORY", DropDownList1.SelectedItem.Value);
      
      lblResult.Text = "You have selected this category. Please select a new category";
    } else {
      SqlCommand cmd1 = new SqlCommand("UPDATE SET CATEGORY CCID@CCID (CATEGORY, C_USERNAME, CCID) VALUES (@CATEGORY, @C_USERNAME, @CCID)", con);
      cmd1.Parameters.AddWithValue("CATEGORY", DropDownList1.SelectedItem.Value);
      cmd1.Parameters.AddWithValue("C_USERNAME", Session["id"]);
      cmd1.Parameters.AddWithValue("CCID", Label1.Text);

      int i = cmd1.ExecuteNonQuery();

      if (i != 0) {

        Label2.Text = " Your data is been saved in the database";
        Label2.ForeColor = System.Drawing.Color.ForestGreen;

      } else {
        Label2.Text = "Something went wrong with selection";
        Label2.ForeColor = System.Drawing.Color.Red;

      }
    }
    con.Close();
  }
}

Now let's discuss this line

    string query = "SELECT CATEGORY FROM CATEGORY WHERE C_UserName = '" + Session["id"] + "'  AND  CATEGORY = '" + DropDownList1.SelectedItem.Value + "' ";

This let's attacker manipulate your input with sql injection. To solve this, use the same cmd1.Parameters.AddWithValue("CATEGORY", DropDownList1.SelectedItem.Value); that you are using in the second query. The Session["id"] is somewhat safer as it is not provided by the user but better safe than sorry as the parameters sanitize the input and protect you from sql injection.

Hi @Athanasios Kataras, thanks for the effort in explaining the way forward. I tried the above code that you provided, unfortunately, there is a error (System.InvalidOperationException: 'There is already an open DataReader associated with this Command which must be closed first.') at the line int i = cmd1.ExecuteNonQuery(); — chimon123, Feb 03 '21 at 14:08

Connection is not closed properly ASP.NET C#

1 Answers1