Make each SQLiteConnection have its own SQLite handle

[dpgao]

Currently all SQLiteConnections share a single underlying SQLite connection stored in an SQLiteDatabase object. This can cause database queries from different threads to be serialised and mixed together, resulting in potential data corruption.

This PR makes each SQLiteConnection have its own SQLite connection handle so that queries from different threads are run on different SQLite connections.

[dpgao]

This new version is causing errors in migrations. The error is

[logging] table "fluent" already exists

I traced it to this error

⚠️ [FluentError.aggregate: The driver closed successfully without a result] [....../Fluent/QueryBuilder/QueryBuilder+Aggregate.swift:114:34] [Suggested fixes: Check the result set count using `count()` before requesting an aggregate.]

I suspect this has to do with this part of the code

https://github.com/vapor/sqlite/blob/ad2e9bc9f0ed00ef2c6a05f89c1cec605467c90f/Sources/SQLite/Database/SQLiteConnection.swift#L87-L100

Changing the last line to the following makes the error disappear. But I doubt if it is the right solution.

            self.eventLoop.execute {
                promise.succeed(result: ())
            }

[dpgao]

Interestingly, this error doesn't occur in the first version where a DispatchQueue is used.

[dpgao]

This bug is now fixed with the latest commit. It turns out that if the event loop in which promise.succeed(result: ()) is run is already self.eventLoop, then the promise would be succeeded before onRow is called. The fix is to force promise.succeed(result: ()) to be scheduled to the event loop.

[dpgao]

In-memory databases can now be shared by several connections. This is achieved by keeping a handle of the in-memory database in the SQLiteDatabase object to keep the in-memory database alive even when all SQLiteConnections are closed.

[dpgao]

@MrLotU Please review the latest additions. Thanks!

[dpgao]

@MrLotU Could you please review the PR? Thank you!

[tanner0101]

This change looks great, thanks! I wish I would have known about the file: prefix before. Just a couple of small comments here. I've also fixed CI to accept forks, so on the next push Circle CI should test this patch.

[tanner0101]

One question, do we need SQLITE_OPEN_FULLMUTEX anymore? SQLiteConnection should only be used on the thread it was created on.

[dpgao]

It is probably not necessary, but I would recommend playing safe and keeping it there...

[dpgao]

@tanner0101 What do you think about using an IUO for storing the handle. This way isClosed simply becomes a computed property.

[tanner0101]

Could we use a regular optional instead? I'd prefer to force unwrap the optional as needed over having that behavior be implicit. I think the C calls might accept normal optionals anyway, though I'm not sure.

[dpgao]

I just tried. The C calls do accept normal optionals, so the compiler won't tell you to force unwrap it.

[tanner0101]

Looks like this patch isn't building on Linux.

...
Compile Swift Module 'SQLBenchmark' (5 sources)
Compile Swift Module 'SQLite' (30 sources)
/root/project/Sources/SQLite/Database/SQLiteConnection.swift:62:16: error: value of type 'UnsafePointer<Int8>' has no member 'map'
        return sqlite3_errmsg(handle).map(String.init(cString:))
               ^~~~~~~~~~~~~~~~~~~~~~ ~~~
error: terminated(1): /usr/bin/swift-build-tool -f /root/project/.build/debug.yaml main output:

Let me know if you need any help debugging this.

[tanner0101]

Thanks a ton for this patch @dpgao! I'm going to do a minor bump on this one since it technically breaks public API by making isClosed get-only. We should really have this bug fix before the next major version though. 👍

[penny-coin]

Hey @dpgao, you just merged a pull request, have a coin!

You now have 1 coins.