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

WIP: Use sync worker #169

Closed
wants to merge 4 commits into from
Closed

Conversation

simolus3
Copy link
Contributor

This offloads the task of synchronizing databases to a separate worker, allowing it to be coordinated across tabs even when the database itself is not in a shared worker.

To test this, compile sync_worker.dart with dart2js and put it into web/ of an example project as powersync_sync.worker.js.
The web database will try to use the shared sync worker, but fall back to the existing implementation if that doesn't work. This uses the new exposeEndpoint method introduced in powersync-ja/sqlite_async.dart#63, giving the sync worker efficient access to the opened database. Requests to upload data and to fetch tokens are forwarded to a connected client.
One thing not yet implemented: The sync worker currently only uses the database connection established by the first tab. When multiple tabs connect to the worker and the first one closes, we'll have to setup a new sync implementation backed by a new tab.

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.

Thanks for the amazing work. From a quick scan this looks great to me. I'll try and test this soon.

Could you update this to target the main branch? main currently contains the latest changes and web support.

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.

This worked well in my tests. In addition to adding better handling for closed tabs, it would be nice if we could broadcast the logs from the Shared Sync Worker to the main context. This makes debugging a lot easier. For example: Currently if there is an authentication error or network error (in the shared sync worker) - the only indication (on the main context) is on the SyncStatus' error field, when the shared sync worker is disabled console.error messages are available in the main context console.

@simolus3 simolus3 changed the base branch from alpha-release to main October 27, 2024 17:49
@simolus3
Copy link
Contributor Author

it would be nice if we could broadcast the logs from the Shared Sync Worker to the main context. This makes debugging a lot easier

Done!

I've also implemented automatic reconnects to another tab if the first tab hosting the sync worker database is closed (this also requires my PR to sqlite_async to forward that information). It's not easy to test in the default configuration because sqlite3_web defaults to using a shared worker with IndexedDB in the default configuration (and the shared connection to a shared worker stays active).

Passing --web-header "Cross-Origin-Opener-Policy=same-origin" --web-header "Cross-Origin-Embedder-Policy=require-corp" to flutter run should make it pick OPFS with separate workers if no IndexedDB database exists alrady. That can be used to test this functionality.
Let me know if there's anything more for me to do here :)

@@ -0,0 +1,258 @@
/// This file needs to be compiled to JavaScript with the command
/// dart compile js -O4 packages/powersync/lib/src/web/sync_worker.worker.dart -o assets/db_worker.js
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a small typo in this command. With the current file structure and default worker path it should be:

dart compile js -O4 packages/powersync/lib/src/web/sync_worker.dart -o assets/powersync_sync_worker.js

We currently compile the DB worker here. I think the sync worker should also be compiled there.

We also need to add this file to our Github releases workflow here.

We have a small CLI tool which devs can use to download a precompiled worker. This sync worker's release file should be added to the tool.

Copy link
Contributor

Choose a reason for hiding this comment

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

@simolus3 You can leave these changes to me. I'll add the respective changes after this PR is merged. This is mainly to prevent merge conflicts with #194 which refactors some of the powersync files to a new package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

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 tested this with the provided flags. Closing tabs seems to work wonderfully :D

@mugikhan mugikhan mentioned this pull request Oct 28, 2024
@mugikhan
Copy link
Contributor

Hey @simolus3 I got around to testing this on my side. When passing these flags --web-header "Cross-Origin-Opener-Policy=same-origin" --web-header "Cross-Origin-Embedder-Policy=require-corp" to flutter run the database updates across tabs does not seem to work. Is it possible for the separate workers to share the database updates?

@simolus3
Copy link
Contributor Author

Hm they should have access to the same files in a sound way, but since they're using independent workers they're also on their own sqlite3 connection / updates stream.

In drift I'm using broadcast channels to work around this issue. I'm not sure where this functionality should live in the powersync stack, I think I can add it to sqlite_async.dart.

@mugikhan
Copy link
Contributor

Hm they should have access to the same files in a sound way, but since they're using independent workers they're also on their own sqlite3 connection / updates stream.

In drift I'm using broadcast channels to work around this issue. I'm not sure where this functionality should live in the powersync stack, I think I can add it to sqlite_async.dart.

Understood, we will continue to merge this as it has support for IndexedDB. We can wait for the syncing with OPFS and separate workers in a future PR.

@simolus3
Copy link
Contributor Author

Understood, we will continue to merge this as it has support for IndexedDB. We can wait for the syncing with OPFS and separate workers in a future PR.

powersync-ja/sqlite_async.dart#74 :)

@mugikhan
Copy link
Contributor

mugikhan commented Nov 1, 2024

Understood, we will continue to merge this as it has support for IndexedDB. We can wait for the syncing with OPFS and separate workers in a future PR.

powersync-ja/sqlite_async.dart#74 :)

Just tested syncing across tabs in chrome using sqlite_async v0.10.1 and it works perfectly!

@simolus3
Copy link
Contributor Author

simolus3 commented Nov 1, 2024

Thanks for you help getting this released 👍

@simolus3 simolus3 closed this Nov 1, 2024
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.

3 participants