sqlite-net: Underlying locking changes have broken async transactions

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

About this issue

Original URL
State: closed
Created 5 years ago
Reactions: 9
Comments: 37 (12 by maintainers)

Commits related to this issue

SimpleWeather: downgrade SQLite.NET * Downgrade back to v1.5.231 * v1.6.292 seems to have lock/mutex issues with async methods * Reference: https://github.com/praeclarum/sqlite-net/issues/881 — committed to SimpleAppProjects/SimpleWeather-NET by thewizrd 4 years ago
Serialize transactions for #881 — committed to praeclarum/sqlite-net by praeclarum 4 years ago
Serialize transactions for #881 — committed to Reddevildragg-UPM-Forks/sqlite-net by praeclarum 4 years ago

Most upvoted comments

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<T> 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 on May 13, 2020

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.

diegostamigni on Apr 27, 2020

@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!

diegostamigni on Apr 27, 2020

Great to hear @pasteusernamehere

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

praeclarum on Aug 20, 2020

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

pasteusernamehere on Feb 3, 2020

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.

gtbX on Nov 12, 2019

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();
      }

gtbX on Nov 11, 2019

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?

pasteusernamehere on Oct 15, 2019