Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature] Concurrent connections & transactions #2

Merged
merged 27 commits into from
Oct 30, 2023

Conversation

stevensJourney
Copy link
Collaborator

@stevensJourney stevensJourney commented Oct 11, 2023

Description

This PR adds the ability to concurrently access a SQLite DB. The work is based off this Blog Post and adapted from https://github.com/journeyapps/sqlite_async.dart.

Multiple read connections and a single write connection are opened in WAL mode. A C++ connection pool wardens which context has access to any of the DB connections at a point in time. Asynchronous queuing is handled via the JavaScript side of JSI bindings. All SQL operations are asynchronously executed in the thread pool.

Single connection DB connections are possible by setting the number of read connections to zero.

Flow

The general flow is:

  • JavaScript requests read/write lock on a connection via readLock/writeLock. JS provides a unique context ID for the lock

    • If a connection is available: The SQLite connection is locked to the connection lock context ID provided by the requestor. The JavaScript bridge is informed from the Connection pool that the requested context lock ID is now active and SQL requests can be made with the context ID (on the relevant connection).
    • If no connections are available, the context ID is added to a FIFO queue. Once other requests are completed the JavaScript bridge is informed that the requested context lock ID is now active.
  • Any SQL requests are triggered (at the correct time) from the JavaScript callback. Those requests are synchronized by returning JSI promises over the bridge.

  • The JavaScript bridge makes a request to release the lock on the connection pool once it's async callback operations are either resolved or rejected.

Tests

The test app was updated and now includes tests for DB connections. All tests currently pass on Android and iOS. An automated test for Android has been added to CI.
image

Related PRs

powersync-ja/powersync-js#7 Updated logic to use new DBAdapter API. Tested in demo application.

linker fix

wired up onContextLockAvailable callback. Cleanup code organization

added requesting of concurrent lock contexts and closing of connections

added ability to release locks

added ability to execute inside lock context

added transactions
@stevensJourney stevensJourney marked this pull request as draft October 17, 2023 14:40
@stevensJourney stevensJourney marked this pull request as ready for review October 24, 2023 11:56
Copy link
Contributor

@rkistner rkistner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are currently still possibilities for concurrency issues.

Taking this example:

await db.writeTransaction(async (tx) => {
   var promises = [];
   for (var i = 0; i < 100; i++) {
      promises.push(tx.executeAsync('SELECT * FROM User'));
   }
   await Promise.all(promises);
});

While this transaction has an exclusive lock on the write connection, the individual statements are concurrently executed on different threads, which can cause issues (not sure exactly what issues, but could be anything from a crash to data corruption). Using SQLITE_OPEN_FULLMUTEX may mitigate those, but then you'd effectively use all the threads for a single transaction in this case, leaving none available for other concurrent transactions.

A good option would be to combine the ThreadPool and ConnectionPool, and have a dedicated thread and work queue per connection - never letting other threads touch the connection.

tests/tests/sqlite/rawQueries.spec.ts Outdated Show resolved Hide resolved
cpp/ConnectionPool.cpp Outdated Show resolved Hide resolved
cpp/ConnectionPool.cpp Outdated Show resolved Hide resolved
cpp/ConnectionPool.cpp Show resolved Hide resolved
src/types.ts Outdated
Comment on lines 126 to 127
execute: (sql: string, args?: any[]) => QueryResult;
executeAsync: (sql: string, args?: any[]) => Promise<QueryResult>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be better to just have a single asynchronous execute method here, same as on the connection.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌 do you think the transaction contexts should only have asynchronous commit and rollback operations as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think it's best if it's all asynchronous. That leaves the option open to change the implementation later, and then same API could also be implemented on other platforms that can only do asynchronous calls.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 The lock and transaction methods are now all async

@stevensJourney
Copy link
Collaborator Author

There are currently still possibilities for concurrency issues.

Taking this example:

await db.writeTransaction(async (tx) => {
   var promises = [];
   for (var i = 0; i < 100; i++) {
      promises.push(tx.executeAsync('SELECT * FROM User'));
   }
   await Promise.all(promises);
});

While this transaction has an exclusive lock on the write connection, the individual statements are concurrently executed on different threads, which can cause issues (not sure exactly what issues, but could be anything from a crash to data corruption). Using SQLITE_OPEN_FULLMUTEX may mitigate those, but then you'd effectively use all the threads for a single transaction in this case, leaving none available for other concurrent transactions.

A good option would be to combine the ThreadPool and ConnectionPool, and have a dedicated thread and work queue per connection - never letting other threads touch the connection.

@rkistner The threadpool is now removed and each connection (inside the connection pool) has it's own thread that will queue operations inside the context lock. This will ensure only one DB operation occurs on a connection at a time. A test was also added for this use case.

Copy link
Contributor

@rkistner rkistner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@stevensJourney stevensJourney merged commit 941194e into main Oct 30, 2023
3 checks passed
@stevensJourney stevensJourney deleted the feature/concurrent-transactions branch October 30, 2023 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants