Need to close the SqlDataReader associated with Command to enable successful login

Question

I'm being presented with the following error message every attempt to login using the correct credentials:

There is already an open DataReader associated with this Command which must be closed first.

Please could someone dissect this code so I can finally move onto the next phase, thanks people!

// Login Function for Manual Login
public void ent()
{
    try
    {
        SqlConnection con = new SqlConnection(cc.connectDB());
        con.Open();
        SqlCommand cmd = new SqlCommand("delete from log", con);
        cmd.ExecuteNonQuery();
        SqlCommand cmd1 = new SqlCommand("select * from Login where username='" + username.Text + "' and password='" + password.Text + "'", con);
        cmd1.ExecuteNonQuery();
        SqlDataReader c = cmd1.ExecuteReader();
        if (c.Read() == true)
        {
            SqlCommand cmd2 = new SqlCommand("select typeid from Login where username='" + username.Text + "' and password='" + password.Text + "'", con);
            Int32 count = (Int32)cmd2.ExecuteScalar();
            if (count == 1)
            {
                SqlCommand cmd3 = new SqlCommand("insert into log values ('" + 1 + "')", con);
                cmd3.ExecuteNonQuery();
            }
            else
                if (count == 2)
                {
                    SqlCommand cmd3 = new SqlCommand("insert into log values ('" + 2 + "')", con);
                    cmd3.ExecuteNonQuery();
                }
            Menu shw = new Menu();
            shw.Show();
            this.Hide();
        }
        else
        {
            MessageBox.Show("Login failed");
        }
    }
    catch (Exception ee)
    {
        MessageBox.Show(ee.Message);
    }
}

I'm connecting to an SQL Express server that is fully up and running but I just can't seem to find a way in which I'm able to close the reader without causing unnecessary errors.

Agree with @Anurag Jain. you have to use finally block and dispose the datareader. — choudhury smrutiranjan parida, Mar 18 '14 at 08:57

Taleeb · Answer 1 · 2014-03-18T09:34:27.080

You are executing select on the same table (dbo.LOGIN) twice without closing the reader before the second execution

Instead of following line

 SqlCommand cmd2 = new SqlCommand("select typeid from Login where username='" + username.Text + "' and password='" + password.Text + "'", con);
 Int32 count = (Int32)cmd2.ExecuteScalar();

You can read typeid as

INT32 count = Convert.ToInt32(c["typeid"].ToString());

Also it is recommended to dispose connection, command and reader objects otherwise you will run into some error sooner or later

For example

using(SqlConnection con = new SqlConnection(cc.connectDB())
  {
    con.Open();
    using(SqlCommand cmd = new SqlCommand("delete from log", con))
      {
            cmd.ExecuteNonQuery();
      }
    using(SqlCommand cmd1 = new SqlCommand("select * from Login where username='" + username.Text + "' and password='" + password.Text + "'", con))
      {
         //You don't need this
         //cmd1.ExecuteNonQuery();
         using(SqlDataReader c = cmd1.ExecuteReader())
              {
                //You don't need these lines - this is probably the line of error
                //SqlCommand cmd2 = new SqlCommand("select typeid from Login where username='" + username.Text + "' and password='" + password.Text + "'", con);
                //Int32 count = (Int32)cmd2.ExecuteScalar();
                INT32 count = Convert.ToInt32(c["typeid"].ToString());

                //other sruffs
              }
       }
  }

In which way can I establish the 'Dispose' command as I keep getting errors every time I place it within my code as a 'Finally' command? — MrBrown, Mar 18 '14 at 09:08
You can surround your connection objects in using block. See the updated answer. — Taleeb, Mar 18 '14 at 09:10
If you have disposed the objects in finally block - then it is fine(you don't need using block). — Taleeb, Mar 18 '14 at 09:13
I used MARS to fix this issue in the end, worked a complete treat! — MrBrown, Mar 19 '14 at 06:26

StuartLC · Answer 2 · 2017-11-21T06:04:54.293

The problem likely originates here, i.e. you should not be calling to an extraneous cmd1.ExecuteNonQuery(); before you invoke cmd1.ExecuteReader():

cmd1.ExecuteNonQuery(); <-- This is redundant, and possibly harmful
SqlDataReader c = cmd1.ExecuteReader(); <-- Just do this.

You just need the call to cmd1.ExecuteReader

However, that said, there are a lot of unclosed / undisposed resources in your code. You should be disposing of connections, commands and readers, e.g. use using:

using (var cmd = new SqlCommand("delete from log", con))
{
   cmd.ExecuteNonQuery();
}

Edit

Where? Like so. And you'll also want to parameterize your queries. Note that you can stack up your usings to prevent a crazy amount of nesting:

  var someMessageToShowToUser = "";
  try
  {
     using (var con = new SqlConnection(cc.connectDB()))
     using (var cmd = new SqlCommand("delete from log", con))
     using (var cmd1 = new SqlCommand(
              "select * from Login where username=@UserName and password=@Password", con))
     {
        con.Open();
        cmd.ExecuteNonQuery();
        cmd1.Parameters.AddWithValue("@UserName", username.Text);
        cmd1.Parameters.AddWithValue("@Password", password.Text);
        // cmd1.ExecuteNonQuery(); -> Delete this line.
        using (var c = cmd1.ExecuteReader())
        {
           if (c.Read() == true)
           {
              using (var cmd2 = new SqlCommand("Parameterize me too")
              {
                 var count = (Int32) cmd2.ExecuteScalar();
                 if (count == 1)
                 {
                    using (var cmd3 = new SqlCommand("insert into log values (@AnotherParam)", con))
                    {
                       cmd3.ExecuteNonQuery();
                    }
                 }
                 else if (count == 2)
                 {
                    using (var cmd3 = new SqlCommand("insert into log values (@AnotherParam)", con))
                    {
                       cmd3.ExecuteNonQuery();
                    }
                 }
                 Menu shw = new Menu();
                 shw.Show();
                 this.Hide();
              }
           }
           else
           {
              someMessageToShowToUser = "Login failed";
           }
        }
     }
 }
 catch (Exception ee)
 {
    someMessageToShowToUser = ee.Message;
 }
 MessageBox.Show(someMessageToShowToUser);

I guess another point is it isn't a good idea to show MessageBoxes (waiting for user to click buttons) while you've got open connections to the database. Ideally you'll want to separate your data access code from your presentation code (and possibly further separations as well).

Thanks for the quick response, where exactly would I insert your recommended code? — MrBrown, Mar 18 '14 at 09:02

Need to close the SqlDataReader associated with Command to enable successful login

2 Answers2