c# System.InvalidOperationException: 'The connection is already open.'

Question

I'm coding a Windows Forms login page for an administration application. My problem is, that when I try to log on, I get the error message

System.InvalidOperationException: 'The connection is already open.'

Any help would be appreciated

public partial class Form1 : Form
{
    MySqlConnection con = new MySqlConnection (@"Database= app2000; Data Source = localhost; User = root; Password =''");
    int i;

    public Form1()
    {
        InitializeComponent();
    }

    private void btnClose_Click(object sender, EventArgs e)
    {
        Application.Exit();
    }

    private void btnLogin_Click(object sender, EventArgs e)
    {
        i = 0;
        con.Open();
        MySqlCommand cmd = con.CreateCommand();
        cmd.CommandType = CommandType.Text;
        cmd.CommandText = "SELECT * FROM adminlogin WHERE username='" + txtBoxUsername + "'AND password='" + txtBoxPassword + "'";
        cmd.ExecuteNonQuery();
        DataTable dt = new DataTable();
        MySqlDataAdapter da = new MySqlDataAdapter(cmd);
        da.Fill(dt);
        i = Convert.ToInt32(dt.Rows.Count.ToString());

        if (i == 0)
        {
            lblerrorInput.Show();
        }
        else
        {
            this.Hide();
            Main ss = new Main();
            ss.Show();
        }
    }
}

Answer 1

Do not cache Connection, it's a typical antipattern, but recreate it when you need it

 public partial class Form1 : Form {
   ...
   //DONE: Extract method
   private static bool UserExists(string userName, string password) {
     //DONE: Do not cache connections, but recreate them
     using (MySqlConnection con = new MySqlConnection (@"...") {
        con.Open();

        //DONE: wrap IDisposable into using
        using (MySqlCommand cmd = con.CreateCommand()) {
          cmd.CommandType = CommandType.Text;

          //DONE: Make query being readable
          //DONE: Make query being parametrized
          cmd.CommandText = 
            @"SELECT * 
                FROM adminlogin 
               WHERE username = @UserName 
                 AND password = @PassWord"; // <- A-A-A! Password as a plain text!

          //TODO: the simplest, but not the best solution: 
          // better to create parameters explicitly
          // cmd.Parameters.Add(...)
          cmd.Parameters.AddWithValue("@UserName", txtBoxUsername);   
          cmd.Parameters.AddWithValue("@PassWord", txtBoxPassword);   

          // If we have at least one record, the user exists
          using (var reader = cmd.ExecuteReader()) {
            return (reader.Read()); 
          }
        }
     }   
   }

Finally

   private void btnLogin_Click(object sender, EventArgs e) {
     if (!UserExists(txtBoxUsername.Text, txtBoxPassword.Text))  
       lblerrorInput.Show();
     else {
       Hide();
       Main ss = new Main();
       ss.Show();
     }
   }

Answer 2

First, don't cache your Connection objects. It's a terrible practice and I've had to go back and fix it every time I accept a new job and inherit code. Most database access classes implement IDisposable, so use using and take advantage of it to keep your code clean. FYI, Readers and Adapters are also IDisposable so you can do the same with them, too.

string command = "select stuff from mydata";
string connection = GetConnectionStringFromEncryptedConfigFile();

    using (var conn = new SqlConnection(connection))
    {
      using (var cmd = new SqlCommand(command, conn))
       {
          cmd.Connection.Open();
          //do stuff            
       }
    }

Second, if you're forced to use a cached connection (i.e., you inherited horrible code and don't have time to fix it yet), check your State first.

if(conn.State != System.Data.ConnectionState.Open)
{
   conn.Open();
}

Note that there are a lot more states than just Open and Closed, and if you try to open a connection that is busy, you'll still get errors. It's still a much wiser approach to use the IDisposable implementations with using so you don't have to worry about this sort of thing so much.

Answer 3

There are some mistakes in your code.

• You should close the sql connection when you finished your process.
• I suggest you to use using statement to dispose connection instance after complete database actions.
• Also, you should use command parameters to prevent Sql injection.

You can declare connection string like this;

private string _connectionString = @"Database= app2000; Data Source = localhost; User = root; Password =''";

The method part looks like;

using (var con = new MySqlConnection(_connectionString))
{
    i = 0;
    con.Open();
    MySqlCommand cmd = con.CreateCommand();
    cmd.CommandType = CommandType.Text;
    cmd.CommandText = "SELECT * FROM adminlogin WHERE username = @username and password = @password";
    cmd.Parameters.AddWithValue("@username", txtBoxUsername);
    cmd.Parameters.AddWithValue("@password", txtBoxPassword);
    cmd.ExecuteNonQuery();
    DataTable dt = new DataTable();
    MySqlDataAdapter da = new MySqlDataAdapter(cmd);
    da.Fill(dt);
    i = Convert.ToInt32(dt.Rows.Count.ToString());

    if (i == 0)
    {
        lblerrorInput.Show();
    }
    else
    {
        this.Hide();
        Main ss = new Main();
        ss.Show();
    }
    con.Close();
}

Answer 4

You forgot to close the connection, use con.Close() at the end to close the connection and avoid this error the next time the event fires.