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

Split out the sliding sync loop into two loops #1961

Closed
Tracked by #1928
poljar opened this issue May 23, 2023 · 2 comments · Fixed by #2023
Closed
Tracked by #1928

Split out the sliding sync loop into two loops #1961

poljar opened this issue May 23, 2023 · 2 comments · Fixed by #2023
Assignees

Comments

@poljar
Copy link
Contributor

poljar commented May 23, 2023

As described in #1928, it would be beneficial if our sync setup involves two loops which don't block each other.

One loop would be for user facing data, the other one for to-device traffic.

The public API should mostly hide the fact that this is happening. We can likely remove the enabling/disabling of the to-device extension from the public API wholesale.

@bnjbvr
Copy link
Member

bnjbvr commented Jun 1, 2023

Some notes from my current implementation for this:

  • it might be required to implement Change the store traits so they expose a transactional API #2000 so we can avoid "one task does partial writes while another task does read" database situations. This could happen because the main loop and the to-device loop would run concurrently.
  • it's not only to-devices, but also the e2ee extension that must run in the second loop.

I've got a naive implementation that mostly reproduces the logic of running a sliding-sync request, that could be refactored. Since these changes are non trivial and would move a lot of code, I think it'd make sense to sync about it before I start doing it:

  • one goal is for the FFI object and public API to not change, to start with.
  • extract the fields from SlidingSync that are responsible of handling a SS request into a SlidingSyncLoop helper: position marker tracking, reset counter
  • extract into "request components" the bits that are specific to each sub-request:
    • for the "main" sync loop, that would be the list of lists (named lists) + sticky parameters
    • for the "to-device" sync loop, that would be another set of sticky parameters
  • now the main loop is a SlidingSyncLoop that has the "main" request components, and another SlidingSyncLoop that has the "to-device" request components
    • future notification process will use a SlidingSyncLoop that enables the to-device component + what it needs for responding to call notifications

Does it make sense @Hywan?

@bnjbvr
Copy link
Member

bnjbvr commented Jun 1, 2023

Reporting from the quick chat we've had with @Hywan: let's keep SlidingSync as is, but make it so that it sends fewer things by requests according to the "role" of the sliding sync, and have an higher-level API with multiple sliding sync for the main app (or hide that in the FFI layer).

bnjbvr added a commit that referenced this issue Jun 19, 2023
This is the first PR for splitting the sync loop into two. This offers a new high-level API, `NotificationApi`, that makes use of a separate `SlidingSync` instance which sole role is to listen to to-device events and e2ee; it's pre-configured to do so. That means we're not force-enabling e2ee and to-device by default for every sliding sync instance, and as such we won't either generate Olm requests to the home server in general.

In the future, this new high-level API will hide some low-level dirty details so that its can be instantiated in multiple processes at the same time (lock across process, invalidate and refill crypto caches, etc.).

An embedder who would want to make use of this would need the following:

- a main sliding sync instance, without e2ee and to-device. Using the `matrix_sdk_ui::RoomList` would be the best bet, at this time.
- an instance of this `matrix_sdk_ui::NotificationApi`, with a different identifier.

Note that this is not ready to be used in an external process; or it will cause the same kind of issues that we're seeing as of today: invalid crypto caches resulting in UTD, etc.

Fixes #1961.
jplatte pushed a commit to matrix-org/matrix-rust-sdk-crypto-nodejs that referenced this issue Jul 12, 2023
This is the first PR for splitting the sync loop into two. This offers a new high-level API, `NotificationApi`, that makes use of a separate `SlidingSync` instance which sole role is to listen to to-device events and e2ee; it's pre-configured to do so. That means we're not force-enabling e2ee and to-device by default for every sliding sync instance, and as such we won't either generate Olm requests to the home server in general.

In the future, this new high-level API will hide some low-level dirty details so that its can be instantiated in multiple processes at the same time (lock across process, invalidate and refill crypto caches, etc.).

An embedder who would want to make use of this would need the following:

- a main sliding sync instance, without e2ee and to-device. Using the `matrix_sdk_ui::RoomList` would be the best bet, at this time.
- an instance of this `matrix_sdk_ui::NotificationApi`, with a different identifier.

Note that this is not ready to be used in an external process; or it will cause the same kind of issues that we're seeing as of today: invalid crypto caches resulting in UTD, etc.

Fixes matrix-org/matrix-rust-sdk#1961.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants