Underlying locking changes have broken async transactions

[pasteusernamehere]

First off, thanks to all the contributors on this project, your effort is greatly appreciated!

Prior to release v1.6.292 I could initiate multiple tasks with db operations and await them all and be guaranteed that each operation would in turn create and commit their own transaction. Tasks would not run simultaneously. It is also worth noting that I am using the default open flags: FullMutex | ReadWrite | Create.

In version 1.5.231 the SkipLock flag was set to true for read operations only. Write operations would then perform a thread lock when performing write operations. This guaranteed that the write operation was the only thread able to start a transaction.

In version 1.6.292 the SkipLock flag is set to true for both read and write operations. When multiple async operations are in play the lock has been changed to use the FakeLockWrapper which does not employ any thread locking. Threads are free to continue down the pipeline to try an initiate a transaction via the write operation.

BeginTransaction uses an Interlocked.CompareExchange to ensure that only one thread can start a transaction. This is good as we only want one transaction at a time, the problem is that if the CompareExchange comparison is not true the code immediately throws an exception (Cannot begin a transaction while already in a transaction). This could be potentially fixed using a semiphore locking mechanism but this also could introduce other issues that I am not aware of. The code could also be fixed by going back to the old locking mechanism employed in v.1.5.231.

Was this behavior change intended? Are consumers now expected to employ their own locking semantics to ensure tasks are queued for execution?

I have attached a small console application that reproduces the issue.

Cheers,

Scott

Code summary:

try
{
    var databasePath = $@"{AppDomain.CurrentDomain.BaseDirectory}test.db";
    var connection = new SQLiteAsyncConnection(databasePath);

    var t1 = Task.Run(async () =>
        await connection.RunInTransactionAsync(db => Thread.Sleep(TimeSpan.FromSeconds(5))));

    var t2 = Task.Run(async () =>
        {
            Thread.Sleep(TimeSpan.FromSeconds(2));
            await connection.RunInTransactionAsync(db => Thread.Sleep(TimeSpan.FromSeconds(5)));
        }
    );

    await Task.WhenAll(t1, t2);
}
catch (InvalidOperationException ex)
{
    Console.WriteLine(ex.Message);
}

SQLiteLockFail.zip

[gtbX]

We're seeing this issue too with the latest release. Setting SkipLock to false is a potential workaround(nope, see below), but I'm unsure as to what the proper solution should be.

[pasteusernamehere]

This may be possible but then all read operations would lock as well. Regardless this seems like a break in the API. The original question still stands.

Was this behaviour change intended? Are consumers now expected to employ their own locking semantics to ensure tasks are queued for execution?

[gtbX]

Setting SkipLock to false is not a valid workaround. Since the SQLiteConnectionPool maintains a weak reference to the underlying connection (and its SkipLock flag) the flag gets reset to true if the GC ever collects the connection.

Currently experimenting with clearing SQLiteOpenFlags.FullMutex, but wary. Dead end, still prone to race conditions.

Whether or not this change is intentional is beyond me, I'm new here 🤷‍♂

[lobbo232]

This could explain why i am seeing SQLite.SQLiteException: database is locked ocuring frequently across users since updating to the latest recently

[LuckyDucko]

I have also noticed this all of a sudden. It seems to randomly show up and cause errors in my app. Just would like to know if this was by design, so I can begin mitigating, or should I downgrade and wait for a fix?

[pasteusernamehere]

For now I am holding off upgrading to this release until some feedback is provided.

[sharathrns]

I am having the same issue after upgrading to version 1.6.292. A multi-thread transactions throws SQLite.SQLiteException: database is locked.
I will be holding off the upgrade for now as well.

[JKennedy24]

I think this could be the real issue I am facing after initially focusing my efforts on this thread:
#700

I too have seen random "Databse is locked" recently and I think it may be linked to this

[diegostamigni]

I had this issue as well and reverted to prev nuget version as my apps were dying with many crash reported. It's also a little scary that @praeclarum isn't replying to any of these issues leaving us the only option to rollback.

[JKennedy24]

This could be potentially fixed using a semiphore locking mechanism but this also could introduce other issues that I am not aware of.

I think this is the best fix, does anyone know how to fix the small sample using Semiphore?

try
{
    var databasePath = $@"{AppDomain.CurrentDomain.BaseDirectory}test.db";
    var connection = new SQLiteAsyncConnection(databasePath);

    var t1 = Task.Run(async () =>
        await connection.RunInTransactionAsync(db => Thread.Sleep(TimeSpan.FromSeconds(5))));

    var t2 = Task.Run(async () =>
        {
            Thread.Sleep(TimeSpan.FromSeconds(2));
            await connection.RunInTransactionAsync(db => Thread.Sleep(TimeSpan.FromSeconds(5)));
        }
    );

    await Task.WhenAll(t1, t2);
}
catch (InvalidOperationException ex)
{
    Console.WriteLine(ex.Message);
}

[gtbX]

We ended up using SemaphoreSlim to lock access to the database in our db access layer, something like this:

    readonly SemaphoreSlim mutex = new SemaphoreSlim(1, 1);
...
      try {
        await mutex.WaitAsync();
        // Similar pattern for other async calls through Connection
        return await Connection.QueryAsync<T>(sql, args);
      } finally {
        mutex.Release();
      }

[JKennedy24]

Heres some helper methods to add to your data access:

        readonly SemaphoreSlim mutex = new SemaphoreSlim(1, 1);

        public async Task RunInMutex(Func<Task> action)
        {
            try
            {
                await mutex.WaitAsync();
                await action.Invoke();
            }
            finally
            {
                mutex.Release();
            }
        }

        public async Task<T> RunInMutex<T>(Func<Task<T>> action)
        {
            try
            {
                await mutex.WaitAsync();
                return await action.Invoke();
            }
            finally
            {
                mutex.Release();
            }
        }

used like:

        var o = await RunInMutex(async () =>
         {
               return await Connection.ExecuteScalarAsync<T>(query, args).ConfigureAwait(false);
          });

I suggest creating a wrapper class for all DataAccess calls

Also be careful not to nest the RunInMutex otherwise you will deadlock it

[JKennedy24]

There is a slight performance impact of locking this way.

Out of interrest, does anyone know if we should we be locking both read and write operations or just write operations?

[gtbX]

Sqlite-net's old behavior created a secondary read-only database connection for reading while the read-write connection was locked. The current implementation uses a single unlocked read-write connection, which is the source of this change in behavior. The secondary connection made it safe to read without locking, since it made it impossible for an async read operation to interfere with a concurrent transaction. That is no longer the case, so yes, you should be locking both read and write operations. Unless, of course, you explicitly open a second read-only connection.

[mchasemd]

Concerning JKennedy24's contribution, I still get a random "database is locked" message. Darn.
Kennedy's code follows:

Heres some helper methods to add to your data access:

    readonly SemaphoreSlim mutex = new SemaphoreSlim(1, 1);

    public async Task RunInMutex(Func<Task> action)
    {
        try
        {
            await mutex.WaitAsync();
            await action.Invoke();
        }
        finally
        {
            mutex.Release();
        }
    }

    public async Task<T> RunInMutex<T>(Func<Task<T>> action)
    {
        try
        {
            await mutex.WaitAsync();
            return await action.Invoke();
        }
        finally
        {
            mutex.Release();
        }
    }

used like:

    var o = await RunInMutex(async () =>
     {
           return await Connection.ExecuteScalarAsync<T>(query, args).ConfigureAwait(false);
      });

[mchasemd]

I spoke too soon. After looking at Kennedy's code again, I added a static to the first line:
static readonly SemaphoreSlim mutex = new SemaphoreSlim(1, 1);
since it was creating a new mutex for each instantiation of my database class. (I also use a single static SQLiteAsyncConnection in the class). Worked like a champ. No more "database is locked" errors.

[pasteusernamehere]

@praeclarum do you care to comment at all on this?

[avorobjovs]

We have the same issue in our apps. We have the "Cannot begin a transaction while already in a transaction" exception after upgrading of the sqlite-net-pcl library to version 1.6.292.

We created a workaround in our code using the SemaphoreSlim class like this:

    public class SemaphoreLocker
    {
        private readonly SemaphoreSlim semaphore = new SemaphoreSlim(1, 1);

        public async Task LockAsync(Func<Task> worker)
        {
            await semaphore.WaitAsync();
            try
            {
                await worker();
            }
            finally
            {
                semaphore.Release();
            }
        }
    }

    public class DbManager
    {
        protected SQLiteAsyncConnection connection { get; private set; }

        private static readonly SemaphoreLocker locker = new SemaphoreLocker();

        public virtual async Task UpdateAsync<TEntity>(string id, TEntity data) where TEntity : class
        {
            await locker.LockAsync(async () =>
            {
                await this.connection.RunInTransactionAsync(...);
            });
        }
    }

It works fine. But we decided to downgrade the sqlite-net-pcl library back to version 1.5.231 before an answer from the author is not provided.

[fgiacomelli]

Same problem for me. I'm using the 1.6.292 version and lately I noticed a lot of worrying errors related to SQLite database.

We use a SQLiteConnectionString with encryption , these SQLiteOpenFlags: SQLiteOpenFlags.ReadWrite | SQLiteOpenFlags.Create | SQLiteOpenFlags.FullMutex
and a BusyTimeout of 10 seconds.

These are the most common problems we are facing it:

• Cannot begin a transaction while already in a transaction.

• SQLiteException: file is not a database
  Error in SQLiteConnection.RunInTransaction

obviously, the problems appears randomly, in a not reproducible way, and we already have done assesments about code (checking that all the transactions don't use the async connection inside) and about permissions

[JKennedy24]

surprising how something so fundamental to modern applications can be so unsupported and broken. Why has no one from Microsoft picked up/forked this project?

[diegostamigni]

surprising how something so fundamental to modern applications can be so unsupported and broken. Why has no one from Microsoft picked up/forked this project?

Maybe because of EF Core which you can pretty much use as a replacement for this.

[JKennedy24]

surprising how something so fundamental to modern applications can be so unsupported and broken. Why has no one from Microsoft picked up/forked this project?

Maybe because of EF Core which you can pretty much use as a replacement for this.

I was wondering whether everyone was using an alternative that had passed me by. This is going to be the 2nd time I've changed my database engine in my project since starting it 5 years ago.

as some background I am developing a Xamarin.forms application

@diegostamigni can you share some links to nugets / github repos and project type you are working on to point me / others in the right direction for alternatives?

[diegostamigni]

@JKennedy24 I've been successfully using sqlite-net for quite a while and I still have apps in production that relies on it. I'm very pissed by this ignoring attitude @praeclarum is having towards this project, which we still love very much (and we thanks him a lot).

That said, I decided to switch to EF Core in a new project of mine I'm developing. This project is a Xamarin Native which for now runs just on iOS. I have 0 problems with EF core, and besides all: it supports migrations out of the box. I know it pretty well as I've been using EF core on my ASP NET Core backends for quite a while now so really a part from installing the SQLite driver (SQLitePclRaw) everything was pretty natural to me.

I used this Medium post as a reference: https://medium.com/@yostane/data-persistence-in-xamarin-using-entity-framework-core-e3a58bdee9d1

You also want to read some material about EF core and understand how it works if you never used it before as it is extremely powerful and can derail you to bad performance and whatnot pretty easily. I suggest reading this book: https://www.manning.com/books/entity-framework-core-in-action but bear in mind is for version 2 of EF core, tho very little has changed in terms of getting started.

Hope this helps!

[fgiacomelli]

surprising how something so fundamental to modern applications can be so unsupported and broken. Why has no one from Microsoft picked up/forked this project?

Good point. I think that it would be desirable that someone from @microsoft take a look about this?

For some developers migrate the current SQLite.Net to EF could be too expensive, not a real solution at this time. We need to find a final fix that solve this concurrent access issue in the shortest possible time

[diegostamigni]

surprising how something so fundamental to modern applications can be so unsupported and broken. Why has no one from Microsoft picked up/forked this project?

Good point. I think that it would be desirable that someone from @microsoft take a look about this?

For some developers migrate the current SQLite.Net to EF could be too expensive, not a real solution at this time. We need to find a final fix that solve this concurrent access issue in the shortest possible time

No one is blocking you to downgrade unless you're working on tvOS (which needs sqlite-pcl-raw 2.0 hence >= 1.6 of this lib). If you rollback to 1.5 everything works fine @fgiacomelli

[JKennedy24]

In terms of solving this particular bug you can either downgrade as @diegostamigni says. Or use this workaround: #881 (comment). Both work. I like the workaround more as it is a bit more defensive against future breaks.

In terms of Microsoft or anyone else taking over maintaining this code. It's more general and looking towards the future. My thoughts are more about when the underlying SqliteRaw libraries change (which they already have to V2 and is only supported by the pre release version of this package), someone needs to update and maintain this code

[fgiacomelli]

Thank you @diegostamigni, my concern is about other bugs that we can face downgrading to that version: me and other persons in my team remember that some times ago there was a certain number of different problems that was solved with latest commits

[diegostamigni]

In terms of Microsoft or anyone else taking over maintaining this code. It's more general and looking towards the future. My thoughts are more about when the underlying SqliteRaw libraries change (which they already have to V2 and is only supported by the pre release version of this package), someone needs to update and maintain this code

Yeah, all true @JKennedy24 and makes sense to me.

Thank you @diegostamigni, my concern is about other bugs that we can face downgrading to that version: me and other persons in my team remember that some times ago there was a certain number of different problems that was solved with latest commits.

As a personal advice @fgiacomelli, I went thru and update my codebase to support #881 (comment) and I have no problem whatsoever. I was also forced to update in order to support my app to run on tvOS so I was really into finding a proper solution. So far so good for me. No issues, been on prod on 4 different platforms for months now.

[csm101]

Hi... I think I solved the problem. I forked the project this morning and made some changes.
See my last commit in https://github.com/csm101/sqlite-net
Did quite some changes:

• now all inter-task locking/synchronization is based on AsyncLock from nuget package Nito.AsyncEx (and on a ObjectWithLock generic class I wrote around it)
• I created a class to which I have delegated the task of tracking the nested transaction level
• I added a specific unit test for the bug we are discussing here

Still to be done:
BusyTimeout: i want it to be considered also as a timeout for obtaining the exclusive access to the SqliteConnection wrapped inside SqliteAsyncConnections, not only as timeout for sqlite low-level database file locking.

If you are interested in giving it a try, any feedback is welcome.

[csm101]

Ok, I just issued a merge request... let's see...

[praeclarum]

Hi everyone, sorry for this trouble. I will investigate today. We have unit tests for async, but I guess they're not good enough.

[praeclarum]

@pasteusernamehere No, a breaking change was not intended. Stability is the most important aspect of this library for me. This was a mistake. I'm sorry it's taken this long for this thread to come to my attention.

[praeclarum]

@fgiacomelli Would you mind opening a new issue for the "SQLiteException: file is not a database
Error"? It seems unrelated to this one but is obviously troubling. I would appreciate any details about what OS you're running, .NET version, etc. since I don't have a repro. Thanks!

[csm101]

@praeclarum Suggestion: I did re-engeneer a lot of things before I touched something that made the bug go away. I have the feeling that the problem lies in the fact that you use lock() and unlock() only if the database is NOT in "full mutex" mode.
The bug went away when started using locking mechanism always, not considering the "full mutex thing".
The problem is that, even if the database does his "full mutex" thing, you check the transaction level BEFORE trying to issue the "start transaction" command, so you don't give the database the chance of locking the caller so you issue immediately the "you can't start a transaction..." error .

[fgiacomelli]

@fgiacomelli Would you mind opening a new issue for the "SQLiteException: file is not a database
Error"? It seems unrelated to this one but is obviously troubling. I would appreciate any details about what OS you're running, .NET version, etc. since I don't have a repro. Thanks!

I just opened an issue #955

[pasteusernamehere]

Sorry for the late feedback but we have now tested the new version and the issue appears to be resolved. Please feel free to close this issue whenever you want.

Cheers and thanks again for your efforts.

[praeclarum]

Great to hear @pasteusernamehere

@csm101 Thank you for the analysis. I'll take another look at it with the next update.