System.Data.SQLite

After upgrading from 1.0.65 to 1.0.83, I'm having a lot of the following exceptions in all my unit tests:

System.IO.IOException : The process cannot access the file 'C:\..path_to_CI_work_folder...\test.db' because it is being used by another process.
	at System.IO.__Error.WinIOError(Int32 errorCode, String maybeFullPath)
	at System.IO.File.InternalDelete(String path, Boolean checkHost)
	at ..test_method..

All my tests usually create a new empty db, fill it up, do some work, then File.Delete the file so that the next unit test will have a clean slate. The File.Delete() fails with the above IO error in 1.0.83, but does NOT fail in 1.0.65

Before jumping to conclusions, yes I use using(..) everywhere and this code is in production on backend services that are up 24/7, using various ADO.NET drivers (MSSQL, Oracle, MySQL, PostgreSQL) with the same pattern. Only SQLiteNET 1.0.83 fails with IO errors. All other tests with other DBs or SqliteNet 1.0.65 work without any problem.

Now, if I add a "GC.WaitForPendingFinalizers(); GC.Collect();" just before the File.Delete() it works properly. This leads me to think that the handle is kept opened by an object still alive in the GC's finalizer queue, and that after the next GC it gets collected and release the lock, but until then it keeps it alive.

After debugging and using a memory profiler for a while, I found that the handle is still opened because there are SQLiteCommand and SQLiteStatement object remanining in the finalizer queue.

Looking at the code, it seems that if you do not explicitly call Dispose() on the SQLiteCommand (or use it inside a using(...)) then it will not release the handle on the file, EVEN if you close the SQLiteConnection and/or use CommandBehavior.CloseConnection

Example of a ExecuteNonQuery that will fail:

		[Test]
		public void Test_SQLite_Disposes_Resources_Properly_After_ExecuteNonQuery()
		{
			string dbPath = @".\test.db";
			if (File.Exists(dbPath)) File.Delete(dbPath);

			// first, create empty db with dummy schema
			using (var f = File.Create(dbPath)) { /* create empty file */ }

			using(var cnx = new System.Data.SQLite.SQLiteConnection("Data Source=" + dbPath))
			{
				cnx.Open();

				var cmd = new System.Data.SQLite.SQLiteCommand("CREATE TABLE foo (bar varchar(255) NOT NULL primary key);", cnx);
				cmd.ExecuteNonQuery();

				// uncomment this to remove the handle leak
				//cmd.Dispose();

			}

			// will fail with IO error if file is still locked
			File.Delete(dbPath);
		}


Example of a ExecuteReader that will fail:

		[Test]
		public void Test_SQLite_Disposes_Resources_Properly_After_ExecuteReader()
		{
			string dbPath = @".\test.db";
			if (File.Exists(dbPath)) File.Delete(dbPath);

			// first, create empty db with dummy schema
			using (var f = File.Create(dbPath)) { /* create empty file */ }

			using (var cnx = new System.Data.SQLite.SQLiteConnection("Data Source=" + dbPath))
			{
				cnx.Open();

				// this is just to create a schema
				var cmd = new System.Data.SQLite.SQLiteCommand("CREATE TABLE foo (bar varchar(255) NOT NULL primary key);", cnx);
				cmd.ExecuteNonQuery();
				cmd.Dispose(); // ensure we dont leak here in order to not mess with the rest of the test

				cmd = new System.Data.SQLite.SQLiteCommand("SELECT * FROM foo;", cnx);
				Assert.AreEqual(ConnectionState.Open, cnx.State, "Should be opened prior to the ExecuteReader(CloseConnection)");
				using (var reader = cmd.ExecuteReader(CommandBehavior.CloseConnection))
				{
					Assert.False(reader.Read());
				}

				// uncomment this to remove the handle leak
				//cmd.Dispose();

				Assert.AreEqual(ConnectionState.Closed, cnx.State, "Should be closed after the ExecuteReader(CloseConnection)");
			}

			// will fail with IO error if file is still locked
			File.Delete(dbPath);
		}


You will probably tell me that I should Dispose the SQLiteCommand, but this pattern is used a lot in my code, and is working fine with 1.0.65, and also with ALL the other ADO.Net drivers that I'm using (SqlClient, Npgsql, MySQL via ODBC, Oracle DAC, Firebird, ...):

public class SimpleDAL
{
    //... some helper methods to create the correct Connection/Command/DataAdapter types depending on the DB used

    public IDbDataReader ExecuteReader(string sqlStatement)
    {
       var cmd = CreateDataCommand(sqlStatement, GetAnOpenedConnectionToTheDb());
       return cmd.ExecuteDataReader(CommandBehabious.CloseConnection);
    }
}

public class SomeClass
{
    private SimpleDAL DAL = .....;
    public void Blah()
    {
       using (var reader = DAL.ExecuteReader("SELECT foo FROM bar ....."))
       {
           // consume the reader
       }

       File.Delete(pathToDbFile); // fails in 1.0.83, not in 1.0.65
    }
}

In this kind of API, I can't return both the SQLiteCommand and the SQLiteDataReader to the caller and have him call Dispose on both.

Also, there are some times where I don't even have the SQLiteCommand object reference, like the override of SQLiteDataAdapter that get the command text as a string and internally create the sql command.

Example of ExecuteDataSet that will leak:

		[Test]
		public void Test_SQLite_Disposes_Resources_Properly_After_ExecuteDataSet()
		{
			string dbPath = @".\test.db";
			if (File.Exists(dbPath)) File.Delete(dbPath);

			// first, create empty db with dummy schema
			using (var f = File.Create(dbPath)) { /* create empty file */ }

			using (var cnx = new System.Data.SQLite.SQLiteConnection("Data Source=" + dbPath))
			{
				cnx.Open();

				// this is just to create a schema
				using (var cmd = new System.Data.SQLite.SQLiteCommand("CREATE TABLE foo (bar varchar(255) NOT NULL primary key);", cnx))
				{
					cmd.ExecuteNonQuery();
				}

				Assert.AreEqual(ConnectionState.Open, cnx.State, "Should be opened prior to the ExecuteDataSet");

				using (var dataSet = new DataSet())
				{
					using (var adapter = new System.Data.SQLite.SQLiteDataAdapter("SELECT * FROM foo;", cnx))
					{
						adapter.Fill(dataSet);

						// uncomment this to remove the handle leak
						//adapter.SelectCommand.Dispose();
					}
				}


			} // should close the connection

			// will fail with IO error if file is still locked
			File.Delete(dbPath);
		}

In the code, the only difference I see between 1.0.65 and 1.0.83 is that SQLiteStatement now has a Finalizer so it will be kept in the finalizer queue for a while (until the next GC).

Using SCITech .NET Memory Profiler, with a snapshot before and after the Dispose() on the SQLiteDataReader, I see that it will not dispose the command because '_disposeCommand' is false

SQLiteDataReader.Close(): (line 130)

          finally
          {
            if (_disposeCommand)
              _command.Dispose();
          }

The only place that will set _disposeCommand to true is inside the Dispose(bool) method of SQLiteCommand itself:

SQLiteCommand.Dispose(bool): (line 205)

                    if (reader != null)
                    {
                        reader._disposeCommand = true;
                        _activeReader = null;
                        return;
                    }

I don't understand how this works ? In order for the DataReader to dispose the command, the command has to be disposed first (to set the flag to true). But then, what is the point of calling _command.Dispose() again since it has already run once ?

I think that if you have a DataReader or DataAdapter that gets Disposed() and it has a CommandBehavior.CloseConnection() it should either Dispose the SQLiteCommand, or at least ensure that it does not keep handles open.

I'm not sure how you can reuse SQLiteCommand object between calls. In my use cases, I always create new SQLiteCommand object just for the request, and do not keep them alive.

Right now, to work around this problem, I'm forcing calls to .Dispose() on the command objects in my DAL code after calling ExecuteReader/ExecuteDataSet, but I'm not sure of the side effects that it will cause If I Dispose the command object and return a still alive DataReader/DataSet to the caller, in all other ADO.NET drivers out there :(

This behavior is by design and is not a bug.

All System.Data.SQLite objects must be explicitly disposed when they are no longer needed, including SQLiteCommand and SQLiteDataReader objects.

If a SQLiteDataReader is returned by a call to the SQLiteCommand.ExecuteReader method, it should normally be disposed prior to disposing of the associated SQLiteCommand.

Also, the locks on the underlying file will not be released until all System.Data.SQLite objects associated with the connection are closed, including the SQLiteConnection object itself.

Well the whole point of a DAL is to provide a clean API where the caller does not have to create the Command/Connection objects (because he does not know the type in advance) and simplify the API (caller does not have to manage connections or state).

Also, if we put aside the DataReader scenario, this does not work with the SQLiteDataAdapter override where the SQLiteCommand object is created by the ctor and not provided by the caller, as in the last example I gave:

    using(var adapter = new System.Data.SQLite.SQLiteDataAdapter("SELECT * FROM foo;", cnx))
    { ... }

Here I never see the Command object, so I can't use the "using(..)" pattern or call Dispose().

The pattern where you only return a SQLiteDataReader or DataSet to the caller, and not the Command object itself, would become possible again, if disposing the Reader would also Dispose() the Command object, at least when the CommandBehaviour is set to CloseConnection.

So to sum up my question is: how can I link the disposing of the command object to the lifetime of the reader or data adapter, when the command object is not exposed to the caller ?

Or at the very least, do you know if calling Dispose on the command after calling ExecuteReader() but before consuming it (calling Read()) would work and would not have any funky side effects ?

It sounds like the issue is in the SQLiteDataAdapter class then.  Looking at the .NET Framework, I can see now that the DbDataAdapter base class does not dispose the command objects that it manages.  I'll implement a workaround in the System.Data.SQLite code.

Fixed on trunk by check-in [42d873a6d8].  All commands associated with the SQLiteDataAdapter object are now explicitly disposed by it (i.e. because the base class does not take care of this).  This should clear the issue you are seeing.

This does indeed solve the problem for SqliteDataAdapter. But I'm still stuck with ExecuteReader. I can't really change my external API because it would break tons of code out there.

I thought of deriving SQLiteDataReader and adding custom dispose logic, but unfortunately it is sealed, and I would prefer not having to create a shim or wrappers around all SQLite ADO.NET classes.

Would you be opposed to exposing the _disposeCommand private member of SqliteDataReader so that users of this type would be able to tell it to Dispose the command when it is Closed ?

So, adding this to SqliteDataReader.cs:

    public bool DisposeCommandOnClose
    {
        get { return _disposeCommand; }
        set { _disposeCommand = value; }
    }

(or a better name for this property, of course)

This way, I could do something like that and keep my external API clean:

    public DbDataReader ExecuteReader(string sqlText)
    {
       DbCommand cmd = SomeHelperThatCreateTheCommand(sqlText);
    
       DbDataReader reader = cmd.ExecuteReader(CommandBehavior.CloseConnection);
       if (reader is System.Data.Sqlite.SQLiteDataReader)
       { // tell SQLiteNetFx to close the command when the reader is closed
           ((System.Data.Sqlite.SQLiteDataReader)reader).DisposeCommandOnClose = true;
       }

       return reader;
    }

and it could still be used by consumers of my API:

    using(var reader = DAL.ExecuteReader("SELECT foo FROM bar"))
    {
        while(reader.Read()) { ... }
    } // <= dispose both reader and command at the same time

It would be kind of hackish, but I can live with it and hide inside some helper method.

Thanks.

There is another solution that will work right now if you are willing to write
code specific to System.Data.SQLite.  You can set the field via Reflection.

For example:

SQLiteDataReader reader = ... code to create a data reader ...

FieldInfo fieldInfo = typeof(SQLiteDataReader).GetField(
    "_disposeCommand", BindingFlags.Instance | BindingFlags.NonPublic);

fieldInfo.SetValue(reader, false);

I could do that, but I'm not sure I like it, because reflection is horribly slow, and this would not durable in regard to future versions (where you could rename/refactor the code).

I'd prefer something that would fail at compile time when the member gets changed, and not horribly crash at 3am on a production server with a cryptic RuntimeException error.

I guess I'll have to bite the bullet, and compile a custom version of SqliteNetFx where I've added this property myself.