System.InvalidOperationException: 'Connection was not closed'

Question

I am working C# Form and SQL Server. I had a problem when login.

"System.InvalidOperationException: 'Connection was not closed'.

I can't solve this problem. I think I add much "con.Open()". But I try much way but I take still this error. Guess, I deleted one more open and close, is it true?

private void buttonLogin_Click(object sender, EventArgs e)
{
    con.Open();

    if (String.IsNullOrEmpty(textBoxUserName.Text))
    {
        MessageBox.Show("Username can't be empty");
        textBoxUserName.Focus();
        con.Close();
    }
    if (String.IsNullOrEmpty(textBoxPassword.Text))
    {
        MessageBox.Show("Password can't be empty");
        textBoxPassword.Focus();
        con.Close();
    }

    else
    {
        con.Open();
        SqlCommand SelectCommand = new SqlCommand("SELECT * FROM Users WHERE  username ='" + textBoxUserName.Text.Trim() + "' and password= '" + textBoxPassword.Text.Trim() + "'");
        SqlDataReader myReader;
        myReader = SelectCommand.ExecuteReader();
        int count = 0;
        string userRole = string.Empty;
        while (myReader.Read())
        {
            count = count + 1;
            userRole = myReader["userrank"].ToString();
        }

        if (count == 1)
        {

            if (userRole =="admin" )
            {
                Form1 form = new Form1();
                this.Hide();
                form.Show();
                con.Close();
            }
            else
            {
                UI ui = new UI();
                this.Hide();
                ui.Show();
                con.Close();
            }
            myReader.Close();
        }
        else
        {
            MessageBox.Show("Check your username or password");
            con.Close();
        }           
    }       
}

Solution: don't reuse your connection but always open and close it where you use it(in the method), best by using the `using`-statement to ensure that it always gets disposed/closed. — Tim Schmelter, Jun 06 '18 at 08:56
You should be using [`using`](https://learn.microsoft.com/dotnet/csharp/language-reference/keywords/using-statement), and you should create (and dispose) a connection in the smallest scope possible, not reuse a `con` object. `SqlConnection` instances are just handles, not physical connections, and you can open and close them with hardly any resource cost. Reusing them will get you nothing but trouble -- for starters, how do you handle broken connections? (The answer is: very awkwardly, if you only have one instance.) — Jeroen Mostert, Jun 06 '18 at 08:57
I do not understand `Form1 form = new Form1();` and `UI ui = new UI();` lines. — SᴇM, Jun 06 '18 at 08:58
You are confusing yourself with so many open/close. Why even bother to open when doing normal validations? Always use `using` so that you are always sure. — Sunil, Jun 06 '18 at 09:00
@SeM If admin want to login, pass Form1 or user want login UI. — Mehmet KUTLU, Jun 06 '18 at 09:06

fubo · Answer 1 · 2018-06-06T09:52:55.780

You could check the state before opening the connection - because opening a open connection will fail.

if(con.State == ConnectionState.Closed)
{
    con.Open();
}

Sidenote: best practice would be

string command = "SELECT * FROM Users WHERE  username = @username and password = @password";
using (SqlConnection con = new SqlConnection(ConnectionString))
{
    con.Open();
    using (SqlCommand cmd = new SqlCommand(command, con))
    {
        cmd.Parameters.Add("@username", SqlDbType.VarChar).Value = textBoxUserName.Text.Trim();
        cmd.Parameters.Add("@password", SqlDbType.VarChar).Value = textBoxPassword.Text.Trim();
        using (SqlDataReader reader = cmd.ExecuteReader())
        {
            while (reader.Read())
            {
                count = count + 1;
                userRole = myReader["userrank"].ToString();
            }
        }
    }
}

with using you don't have to care about state, closing and disposing the connection
use parameters to avoid injection attacks

loeje · Answer 2 · 2018-06-06T09:31:11.783

It seems like you have some redundant code. There's no need to open the connection at the beginning and closing it again later without have to use the connection.

Also it seems like it might hit your else statement if it hits your first if.

I think the code below would solve your issue where the con object could be closed by hitting the first if statement. Assuming you've instantiated your con object globally of course.

private void buttonLogin_Click(object sender, EventArgs e)
{
    if (String.IsNullOrEmpty(textBoxUserName.Text))
    {
        MessageBox.Show("Username can't be empty");
        textBoxUserName.Focus();
    }
    else if (String.IsNullOrEmpty(textBoxPassword.Text))
    {
        MessageBox.Show("Password can't be empty");
        textBoxPassword.Focus();
    }
    else
    {
        using (SqlConnection con = new SqlConnection(ConnectionString))
        {
            SqlCommand SelectCommand = new SqlCommand("SELECT * FROM Users WHERE  
            username ='" + textBoxUserName.Text.Trim() + "' and password= '" + textBoxPassword.Text.Trim() + "'");
            SqlDataReader myReader;
            myReader = SelectCommand.ExecuteReader();
            int count = 0;
            string userRole = string.Empty;
            while (myReader.Read())
            {
                count = count + 1;
                userRole = myReader["userrank"].ToString();
            }

            if (count == 1)
            {

                if (userRole =="admin" )
                {
                    Form1 form = new Form1();
                    this.Hide();
                    form.Show();
                    con.Close();
                }
                else
                {
                    UI ui = new UI();
                    this.Hide();
                    ui.Show();
                    con.Close();
                }
                myReader.Close();
            }
            else
            {
                MessageBox.Show("Check your username or password");
                con.Close();
            }           
        } 
    }      
}

I try it but now i take this: "System.InvalidOperationException: 'Connection dont starting" — Mehmet KUTLU, Jun 06 '18 at 09:25
I've edited my answer with inspiration from @fubo regarding the "using" statement. You have to move the global instantiation of the "con" object to the "using" line. — loeje, Jun 06 '18 at 09:32

score 0 · Answer 3 · answered Jun 06 '18 at 09:21

0

use using construct.

using (con)
{
  //do your work
}

This will automatically dispose connection in case of success and exception

answered Jun 06 '18 at 09:21

Learner

1,277
3
15
34

`con` should also be declared within using to dispose correctly `using (SqlConnection con = new SqlConnection(ConnectionString))` – fubo Jun 06 '18 at 13:09

System.InvalidOperationException: 'Connection was not closed'

3 Answers3