I get this error: database table is locked

[IliaPreston]

I get a very strange error.
Here is my code:

        private void btnTest011_Click(object sender, EventArgs e)
        {
            int i;
            int RecCnt;
            string sql;
            string sqlIns;
            string sqlRecExists;
            SQLiteCommand cmdIns;
            SQLiteCommand cmd;
            SQLiteDataReader rdr;

            btnTest011.Enabled = false;

            //sql = @"drop  table  if exists  TMP_STUDENTS";
            //cmd = new SQLiteCommand(sql, conn);
            //cmd.ExecuteNonQuery();


            sql = @"create  temp table  if not exists  TMP_STUDENTS
                       as  select  *   from  STUDENTS
                            where  1 = 2";
            cmd = new SQLiteCommand(sql, conn);
            cmd.ExecuteNonQuery();


            for (i = 1; i <= 3; i++)
            {
                sql = "insert  into  TMP_STUDENTS (STD_FNAME, STD_LNAME, STD_NUM)  values  ('John" + i + "', 'Davis', 1001)";
                cmd = new SQLiteCommand(sql, conn);
                cmd.ExecuteNonQuery();
            }


            sql = "";
            sql = sql + "      select   STD_FNAME, STD_LNAME, STD_NUM ";
            sql = sql + "        from  TMP_STUDENTS ";
            sql = sql + "       order  by  STD_NUM  asc";

            cmd = new SQLiteCommand(sql, conn);
            rdr = cmd.ExecuteReader();

            string StdFname;
            string StdLname;
            int StdNum;


            while (rdr.Read())
            {
                StdFname = "" + rdr["STD_FNAME"];
                StdLname = "" + rdr["STD_LNAME"];
                StdNum = int.Parse("" + rdr["STD_NUM"]);

                sqlRecExists = "select count(*) from STUDENTS  where STD_NUM = " + StdNum;
                RecCnt = RunSqlReturnInt(sqlRecExists);

                if (RecCnt == 0)
                {
                    sqlIns = "";
                    sqlIns = sqlIns + "insert into  STUDENTS ( STD_FNAME, STD_LNAME, STD_NUM ) ";
                    sqlIns = sqlIns + "      values     ( '" + StdFname + "', '" + StdLname + "', " + StdNum + ")";

                    cmdIns = new SQLiteCommand(sqlIns, conn);
                    cmdIns.ExecuteNonQuery();
                }

            }


            sql = @"drop  table  if exists  TMP_STUDENTS";
            cmd = new SQLiteCommand(sql, conn);
            cmd.ExecuteNonQuery();

            btnTest011.Enabled = true;

        }

The error occurs on the red line.
Here is the error message:

database table is locked

I have tried to track down the cause of the error, and eventually I found out that 17 lines above that red line, I have this:

                RecCnt = RunSqlReturnInt(sqlRecExists);

If I remove that line and replace it with this:

                RecCnt = 1;

Then the error does not happen, but then the logic of the program is not correct.

That method is defined as follows:

        public int RunSqlReturnInt(string sql, int AltValue = 0)
        {
            SQLiteCommand cmd;
            SQLiteDataReader rdr;

            cmd = new SQLiteCommand(sql, conn);
            rdr = cmd.ExecuteReader();

            if (rdr.RecordsAffected == 0)
            {
                return AltValue;
            }
            else
            {
                while (rdr.Read())
                {
                    return rdr.GetInt32(0);
                }
            }
            return AltValue;
        }

The connection (that is conn) is declared at form level:

namespace Test001SqLite
{
    public partial class Form1 : Form
    {
        public Form1()
        {
            InitializeComponent();
        }

        SQLiteConnection conn;


        public int RunSqlReturnInt(string sql, int AltValue = 0)

Every other thing (command objects, reader objects, etc. are local to the methods that they are used.

I cannot understand why this error occurs.
And how to fix it.
Can you please help?
Thanks.

[jmcilhinney]

You're not disposing your data reader. You execute a query and open a data reader and never close it, so the table it is reading is locked. That RunSqlReturnInt method is rubbish anyway. You shouldn't be calling ExecuteReader at all. The ExecuteScalar method exists specifically to execute a query and retrieve a single value, so that's what you should be calling. If you were going to use a data reader though, you should create it with a 'using' statement, so that it will be disposed and thus closed at the end of the block:

using (var reader = command.ExecuteReader())
{
    // Use reader here.
} // reader is implicitly disposed here.

I don't see you closing your connection anywhere either, so that's also bad. You should not really be using a common connection object to start with, but you definitely shouldn't be keeping it open all the time. You should be creating and opening the connection object where you need to use it, then closing and discarding it when you're done with it. That's how ADO.NET is designed to be used. You might like to check out my ADO.NET code examples here.

[IliaPreston]

You're not disposing your data reader. You execute a query and open a data reader and never close it, so the table it is reading is locked. That RunSqlReturnInt method is rubbish anyway. You shouldn't be calling ExecuteReader at all. The ExecuteScalar method exists specifically to execute a query and retrieve a single value, so that's what you should be calling. If you were going to use a data reader though, you should create it with a 'using' statement, so that it will be disposed and thus closed at the end of the block:

using (var reader = command.ExecuteReader())
{
    // Use reader here.
} // reader is implicitly disposed here.

I don't see you closing your connection anywhere either, so that's also bad. You should not really be using a common connection object to start with, but you definitely shouldn't be keeping it open all the time. You should be creating and opening the connection object where you need to use it, then closing and discarding it when you're done with it. That's how ADO.NET is designed to be used. You might like to check out my ADO.NET code examples here.

Thanks a lot for your help.
I have a few questions if you don't mind.
I would appreciate it if you could reply to them:

1- You are talking about disposing the reader.

The reader does not have a Dispose method.
It however has a Close method.
When you talk about disposing a reader, do you mean rdr.Close()?
I guess yes, but, I am asking because I don't want to leave anything to chance.

2- I added the rdr.Close() to RunSqlReturnInt, and suddenly the error disappeared and the program started to work.

However, if you look at the main method (that is btnTest011_Click), in there too, I don't dispose the reader.
How come THAT doesn't cause the same error (for example if I remove the call to RunSqlReturnInt from btnTest011_Click)?

3- You are saying:

ou're not disposing your data reader. You execute a query and open a data reader and never close it

But, when the reader goes out of scope (that RunSqlReturnInt exits), doesn't the reader get disposed automatically?
Please correct me if I am wrong: As far as I understand, everything that goes out of scope is disposed. Isn't that true?

4- You are saying:

using (var reader = command.ExecuteReader())
{
    // Use reader here.
} // reader is implicitly disposed here.

Why is the reader disposed there automatically?
What is the general rule?
You are saying that the reader is disposed automatically here because you know the general rule as to when the reader is disposed automatically and when it is not.
Please tell me what that general rule is.

Thanks a lot for your help.

[jmcilhinney]

1. The data reader does have a Dispose method because it implements IDisposable. It is an explicit implementation though, so you'd have to cast as type IDisposable in order to call it directly. Many disposable types have a Close method because it is more natural than Dispose, so you can call Close directly and it will call Dispose internally. You should be creating the object with a using stament though, in which case there's no need to explicitly close or dispose.

2. I'm not sure but I would expect that it's because in one case you are only calling Read once and returning, so you never actually call Read and get false returned. In the other case, you keep calling Read until it returns false. My guess would be that the table(s) being read is locked until you get that false return value, at which point the system knows that all the data has been read. There's probably a cursor being held on the database until that point.

3. Going out of scope is something that happens to a variable/reference, not an object. It should be fairly obvious that a variable going out of scope can't dispose the object that it refers to, because that object could be being referred to by one or more other variables and still be being used elsewhere. Disposing an object is something that you must do yourself, either directly or via a using block. If a variable goes out of scope then that's one less reference to that object. If that was the last reference then the object becomes eligible for cleanup by the GC but that cleanup may not happen for some time, so the object will remain in its current state until then.

4. That's how a using statement works. That's what it's for. You should read the documentation for the using statement if you want to know more details. Basically, the using statement creates a scope for a variable and the Dispose method of that variable is guaranteed to be called at the end of that scope, even if an exception is thrown. It's a succinct way to ensure that objects that need it are disposed without having to write try...finally blocks and dispose the objects explicitly.

[IliaPreston]

Thanks a lot for your help.

Basically, I understand that using the "using" statement is the best, and I am going to adopt that way of coding.
However, before I abandon my current way of coding and use the "using" statement, I would like to understand something:

You are saying:

1. The data reader does have a Dispose method because it implements IDisposable. It is an explicit implementation though, so you'd have to cast as type IDisposable in order to call it directly. Many disposable types have a Close method because it is more natural than Dispose, so you can call Close directly and it will call Dispose internally.

Please correct me if I am wrong: You are saying that instead of Dispose, I can use the Close method and that is equally as good as the Dispose method.
is that right?


Also, I have tested and I know that all three of the below codes work (please see the red lines):
A)

        private void btnTest012_Click(object sender, EventArgs e)
        {
            string sql;
            SQLiteCommand cmd;
            SQLiteDataReader rdr;

            txtDetails.Text = "";

            sql = "select count(*) from STUDENTS  where STD_NUM = 1001";
            cmd = new SQLiteCommand(sql, conn);
            rdr = cmd.ExecuteReader();

            while (rdr.Read())
                txtDetails.Text = "" + rdr.GetInt32(0);

            rdr.Close();
        }

B)

        private void btnTest012_Click(object sender, EventArgs e)
        {
            string sql;
            SQLiteCommand cmd;
            SQLiteDataReader rdr;

            txtDetails.Text = "";

            sql = "select count(*) from STUDENTS  where STD_NUM = 1001";
            cmd = new SQLiteCommand(sql, conn);
            rdr = cmd.ExecuteReader();

            while (rdr.Read())
                txtDetails.Text = "" + rdr.GetInt32(0);

            ((IDisposable)rdr).Dispose();
        }

C)

        private void btnTest012_Click(object sender, EventArgs e)
        {
            string sql;
            SQLiteCommand cmd;
            SQLiteDataReader rdr;
            IDisposable idsp;

            txtDetails.Text = "";

            sql = "select count(*) from STUDENTS  where STD_NUM = 1001";
            cmd = new SQLiteCommand(sql, conn);
            rdr = cmd.ExecuteReader();
            idsp = (IDisposable)rdr;

            while (rdr.Read())
                txtDetails.Text = "" + rdr.GetInt32(0);

            idsp.Dispose();
        }

Which one of the above three approaches is better and why?
Please advise.
Thanks.

[jmcilhinney]

Here is some of the source code from the DbDataReader class, which all data reader classes are derived from:

        virtual public void Close()
        {
        }
 
        [
        EditorBrowsableAttribute(EditorBrowsableState.Never)
        ]
        public void Dispose() {
            Dispose(true);
        }
    
        protected virtual void Dispose(bool disposing) {
            if (disposing) {
                Close();
            }
        }

As you can see, the Close method is virtual (Overridable) and the Dispose method calls Close. Individual data reader classes, like the one you're reading, will pretty much always override the Close method but not the Dispose method. If you call Dispose, it's going to call Close anyway, so they amount to the same thing.

As for your three code snippets, they all effectively do the same thing so none is better than the others from that perspective. The first one is obviously less complex and more readable though, so there's little reason to not use that one, if you couldn't use a using statement for some reason.

[IliaPreston]

Thanks a lot for your help.

Regarding my code, and that I am going to get rid of it and instead use the "using" statement, I need to understand one thing.
You said:

That RunSqlReturnInt method is rubbish anyway

Well, I now know that that routne had a major flaw (it didn't dispose the reader).
Now, if I correct that code like this:

        public int RunSqlReturnInt(string sql, int AltValue = 0)
        {
            int RetVal;
            SQLiteCommand cmd;
            SQLiteDataReader rdr;

            cmd = new SQLiteCommand(sql, conn);
            rdr = cmd.ExecuteReader();

            if (rdr.RecordsAffected == 0)
            {
                rdr.Close();
                return AltValue;
            }
            else
            {
                while (rdr.Read())
                {
                    RetVal = rdr.GetInt32(0);
                    rdr.Close();
                    return RetVal;
                }
            }
            rdr.Close();
            return AltValue;
        }

then, is it still bad?
Is there any other problem with it?

Please don't get me wrong. I am not trying to defend my bad code or add some band-aid solutions to it in order to keep it.
I am definitely going to get rid of it and use the "using" statement instead.
But, before abandoning it, I just need to understand what makes that code so bad.
When you say "That RunSqlReturnInt method is rubbish anyway", that word "anyway" means:
There are other things wrong with that code, other than the fact that it fails to dispose the reader.
So, in what other ways is that code wrong or bad?

Please advise.
Thanks a lot.

[jmcilhinney]

As I said at the time, if you're getting a single value from a result set then you should not be using a data reader in the first place.

public int GetInt32(string sql, int defaultValue = 0)
{
    using (var connection = new SQLiteConnection(connectionString))
    using (var command = new SQLiteCommand(sql, connection))
    {
        connection.Open();

        var result = command.ExecuteScalar();

        return result == DBNull.Value
                   ? defaultValue
                   : (int)result;
    }
}

[IliaPreston]

As I said at the time, if you're getting a single value from a result set then you should not be using a data reader in the first place.

public int GetInt32(string sql, int defaultValue = 0)
{
    using (var connection = new SQLiteConnection(connectionString))
    using (var command = new SQLiteCommand(sql, connection))
    {
        connection.Open();

        var result = command.ExecuteScalar();

        return result == DBNull.Value
                   ? defaultValue
                   : (int)result;
    }
}

Thanks a lot for your help.

Is that method GetInt32 in the above quote what you use to retrieve a single integer value?
And do you recommend it?

Thanks.