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

Prototype for impl with sqlite3_web package #27

Closed

Conversation

simolus3
Copy link
Contributor

This demonstrates how an async wrapper implementing interfaces from this package could look like when using the new shared sqlite3_web package.
That package extracts most of the logic currently part of drift to detect browser features and to host sqlite3 databases in a web worker.

I only had a quick look at this package and so I didn't integrate this with the open factories yet, but still it implements SqliteDatabase and should support running statements and receiving update notifications. Update notifications are shared between tabs when using AccessMode.throughSharedWorker.
The sqlite3_web package will provide a mechanism for custom requests. Drift will use that to broadcast query updates since it's not using native update notifications at the moment. sqlite_async can use that to implement locks, by making clients issue a lock request before accessing the database. This functionality is not yet implemented in sqlite3_web though, once that's done I can integrate that here.
Another thing not yet implemented is the automated detection of a suitable VFS implementation or worker setup based on supported browser features. Browser features can already be detected though, so it's just a matter of using that information.

I've seen that there is an existing implementation for local databases with SyncSqliteConnection. We may have to use that if the browser doesn't support workers at all, of if we're using per-tab IndexedDB where using workers doesn't make much of a difference. But since drift will have to do the same thing, it's also something that could be implemented in the sqlite3_web package too.
For scenarios in which different tabs don't share the actual sqlite3 connection through a shared worker (for instance because shared workers aren't available), things like update notifications would have to be broadcast using another mechanism (I'm using currently using the Broadcast Channel API for that in drift). I'm not sure if that part should be implemented in sqlite3_web as well since at that point we really have multiple "processes" accessing the same sqlite database with no shared-memory between them.

WebDatabase() {
_initialize = Future.sync(() async {
final sqlite3 = await WebSqlite.open(
wasmModule: Uri.parse('todo: how to specify wasm uri'),
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be passed in via an SqliteOptions parameter. The DB filename could be passed in via path (for consistency). An example of the typical constructor can be found here.

@rkistner
Copy link
Contributor

Thanks, it looks great so far!

I only had a quick look at this package and so I didn't integrate this with the open factories yet

We're busy refactoring those to simplify things a little - so don't worry about that just yet.

We'll hopefully have more feedback on the other details later this week / early next week.

@simolus3
Copy link
Contributor Author

simolus3 commented Mar 4, 2024

Did you have a chance to look at this yet? There's still work for me to do in the shared package, but if there is a fundamental limitation of the approach/API that doesn't work for you, it would be good to know before porting everything else over.

@stevensJourney
Copy link
Contributor

Did you have a chance to look at this yet? There's still work for me to do in the shared package, but if there is a fundamental limitation of the approach/API that doesn't work for you, it would be good to know before porting everything else over.

Hi @simolus3 ! We've made some changes to the structure for opening connections here #30.

The SqliteDatabase web implementation now uses the DefaultSqliteOpenFactory from lib/src/web/web_sqlite_open_factory.dart's openConnection method to create its primary asynchronous database connection.

SqliteDatabase (on web) is now essentially a thin wrapper for the SqliteConnection which is returned from the open factory it is constructed with.

It should not take much effort to move this PR implementation to the DriftSqliteConnection.

@simolus3
Copy link
Contributor Author

Sorry for the delay on this - I've moved the implementation over to the sqlite3_web package now. When the package uses a shared worker, we can avoid the local lock management and coordinate that through the shared worker hosting the database. We're still using the existing mutex for connections made through dedicated workers.

Are there any tests or working examples I could use to test the web implementation?

@stevensJourney
Copy link
Contributor

Sorry for the delay on this - I've moved the implementation over to the sqlite3_web package now. When the package uses a shared worker, we can avoid the local lock management and coordinate that through the shared worker hosting the database. We're still using the existing mutex for connections made through dedicated workers.

Are there any tests or working examples I could use to test the web implementation?

This looks amazing so far. There should be some automated tests for web which you can verify with. This example app is a bit upstream, it uses the powersync.dart package which links to this sqlite_async.dart package ,but we have a working example of a todolist Flutter app here

You should be able to test using the supabase-todolist demo in the demos folder. The README should contain all the instructions required to get that demo up and running. I suspect there might be some issues if getAutoCommit is not yet implemented.

@simolus3 simolus3 marked this pull request as ready for review April 21, 2024 21:02
Copy link
Contributor

@stevensJourney stevensJourney left a comment

Choose a reason for hiding this comment

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

I did some testing and after some minor tweaks on the sqlite_async.dart and upstream ends on our side I think that the new SQLite packages are working perfectly.

I had some questions in comments and other notes. I think we can action the note items for sqlite_async.dart on our end, they are only on this PR since it's easy to keep track of them. Feel free to comment if you have any feedback on those notes.

From my side I would be happy if the new packages could be published, then we can use them and make further changes :)

}
}

class _ExlusiveContext extends _SharedContext implements SqliteWriteContext {
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: This contains a typo :)

}

@override
Future<T> readTransaction<T>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: This test currently is failing due to not throwing an exception if a SQL statement is executed after the transaction has been rolled back. This used to work by checking the state of autoCommit here. This does unfortunately perform a call to getAutoCommit for each executed statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the semantics you need are "execute if in transaction, otherwise throw", this should be possible with custom requests. _AsyncSqliteDatabase.handleCustomRequest has access to the raw database and could implement this request within a single round-trip if necessary.


@override
Future<Row> get(String sql, [List<Object?> parameters = const []]) async {
final results = await getAll(sql, parameters);
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: Operations in these contexts should throw instances of SqliteException if it's a corresponding exception. This is used in places like here. This was previously implemented here.

final class _AsyncSqliteController extends DatabaseController {
@override
Future<WorkerDatabase> openDatabase(WasmSqlite3 sqlite3, String vfs) async {
final db = sqlite3.open('/app.db', vfs: vfs);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just want to confirm my understanding of paths here. From testing it seems like the path parameter is used for the VFS identifier (noticed the IndexDB is named after path) and the /app.db is the virtual file inside the VFS.
Since the path is used for the IndexDB DB name, then multiple DBs should be supported?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes exactly, but this should definitely be documented upstream. And this behavior is actually broken, the path should be /database.

The reason for a VFS-level database isolation is that some VFS implementations only support a fixed set of files. In particular, the file system access API through OPFS gives us synchronous IO (neat to implement VFS without worker hacks or a buffer with an async queue afterwards), but we can't open files synchronously. So if we open /database and /database-journal before registering the VFS, we have a working synchronous VFS in the browser.

I'll change the implementation upstream to provide the correct path to openDatabase, the current situation is just horrible.

WebSqlite.workerEntrypoint(controller: _AsyncSqliteController());
}

final class _AsyncSqliteController extends DatabaseController {
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: sqlite_async.dart might need to export AsyncSqliteController and AsyncSqliteDatabase classes in order for consumers of the package to extend this in custom workers. For example if additional custom functions need to be registered.


@override
// todo: Why do we have to expose both a stream and a controller?
StreamController<UpdateNotification> get updatesController =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: It should not be necessary to override this. This was added to SqliteDatabaseMixin since we used it for both web and native. But this could be moved from SqliteDatabaseMixin to reside solely in the native implementation.

import 'dart:js_interop';

/// Custom function which exposes CommonDatabase.autocommit
const sqliteAsyncAutoCommitCommand = 'sqlite_async_autocommit';
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: I think this can be removed now.

/// Web implementation of [AbstractDefaultSqliteOpenFactory]
class DefaultSqliteOpenFactory
extends AbstractDefaultSqliteOpenFactory<CommonDatabase> {
// todo: For users with multiple databases, the WebSqlite should be shared
Copy link
Contributor

Choose a reason for hiding this comment

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

How would this look like?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A single WebSqlite instance can handle multiple databases, but every WebSqlite instance potentially spawns its own worker so having more than one should be avoided.

If I remember correctly, we're currently using a new DefaultSqliteOpenFactory for each database (and it has to be like that because the path is bound to the factory). So perhaps we could have a static Map<SqliteOptions, Future<WebSqlite>> around that lazily creates a new WebSqlite instance only if a different URI for the wasm module or the worker is used. Otherwise the existing instance can be re-used.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense. We can do that on our end.

@simolus3
Copy link
Contributor Author

I've published the first version of sqlite3_web to pub.dev

@stevensJourney
Copy link
Contributor

stevensJourney commented May 15, 2024

I've published the first version of sqlite3_web to pub.dev

Thanks! Will try and test. Are the sqlite3 package changes also published? I see it is already published.

@stevensJourney stevensJourney mentioned this pull request May 22, 2024
@stevensJourney
Copy link
Contributor

@simolus3 I implemented some of the notes on this PR here, in testing that work and some additional upstream testing I noticed that calling the close method seems to throw an exception.
image
Here is an example of where breaking could occur.
Do you have any ideas of what would be causing this error?

@simolus3
Copy link
Contributor Author

Thanks, the problem was that we first ask the worker to close the database and then close the stream controller. Once the cancel callback from the stream controller for updates gets invoked, we ask the worker to stop sending us update notifications for that database. The worker then gets confused since the database we're asking about doesn't exist.

I've fixed this in version 0.1.1-wip of sqlite3_web.

@stevensJourney
Copy link
Contributor

Thanks, the problem was that we first ask the worker to close the database and then close the stream controller. Once the cancel callback from the stream controller for updates gets invoked, we ask the worker to stop sending us update notifications for that database. The worker then gets confused since the database we're asking about doesn't exist.

I've fixed this in version 0.1.1-wip of sqlite3_web.

Thanks for the update @simolus3 . I tested the new packages and the close method no longer throws an exception. I did notice that it seems like the DB is cleared after closing a connection. Is that expected? I added a test for this here. I have confirmed that the same path is being used for both connections, but maybe I'm missing something silly.

@simolus3
Copy link
Contributor Author

It was my silly mistake - despite IndexedDB being available, sqlite3_web determined that it should use an in-memory database. simolus3/sqlite3.dart@cc09578 fixes that, I've released it as 0.1.2.

@simolus3
Copy link
Contributor Author

Hi, I'm wondering if you have additional feedback or things you need from sqlite3_web here? If the current API matches what you have in mind and works for you, I'll start working on integration tests on the sqlite3_web side to start finishing this up.

@stevensJourney
Copy link
Contributor

Hi, I'm wondering if you have additional feedback or things you need from sqlite3_web here? If the current API matches what you have in mind and works for you, I'll start working on integration tests on the sqlite3_web side to start finishing this up.

Hi @simolus3 . Sorry for the late reply. I think we have everything we need in the current API. We've just merged the work from this PR in this PR here.. I'll close this pull request, since your work is included there. Thank you for the wonderful library!

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.

None yet

3 participants