This file claimed to have lots of tests, but actually they are not independent
- they are just one big test.
It's not ideal to have one massive test like this, but I don't really have time
to rewrite them and we should stop pretending.
and the latest few encrypted events in Room.
Use the latest event to provide a room preview, and use the encrypted
events to replace the laest event when they are decrypted.
This patch introduces a new `chunk_large_query_over` function. Imagine
there is a _dynamic_ query that runs potentially large number of
parameters, so much that the maximum of parameters can be hit. Then,
this function is for you. It will execute the query on chunks of
parameters.
`chunk_large_query_over` uses `Vec::split_off` to avoid cloning `Key`s
as much as possible. It's difficult to use references here because of
the async nature of `SqliteObjectExt::prepare`.
This patch updates `get_kv_blobs`, `get_room_infos`,
`get_maybe_stripped_state_events_for_keys`, `get_profiles`,
`get_user_ids`, and `get_display_names` to use this new function.
This patch finally adds the `repeat_vars` function to replace in a
more efficient way the `vec!["?"; n].join(", ")` pattern. There is less
memory allocations.
The `RoomList` and `EncryptionSync` API provide a better developer
experience and address concrete needs. Nobody no longer uses the Sliding
Sync bindings, so we can remove them.
For sliding sync that starts both the actual sliding sync request along the e2ee requests,
we need to make sure that, in case of failure of sliding sync, we reset `pos` as soon as possible.
With the code before this patch, the sliding sync response might be available, but the whole
processing could be waiting for the e2ee requests to finish. In the updated version, we spawn
the e2ee requests in a background task, then run the sliding sync request immediately and fail
if it failed.
Another nice benefit is that the e2ee requests won't be interrupted in the middle of processing,
if the sliding sync changed parameters and we cancelled the whole sync a bit too early.
Fixes#2206.
`RoomListServer` defines an `all_room` sliding sync list. This list
starts in selective sync-mode, then it switches to growing sync-mode.
The previous batch size of the growing sync-mode was 50. This patch
updates it to 200, because empirically it seems a better value for
perceived performance.
This patch also rewrites how `State::next` is written. No change in the
code, just comestic.
* sdk: Allow to update notification settings
* Add an event listener for push rule events
* Fix: simplify insertion and deletion of rules
* Fix: Limit rules cloning
* Fix: Unit tests
* Fix: set_room_notification_mode
* Fix: potential race condition when updating the local ruleset
* Refactor RuleCommands
* RuleCommands Unit Tests
* Fix: limit lock usage
* nit: use expression assignment by default
* nit: pass Ruleset by ownership in RuleCommands' ctor
* a few nits: use to_owned() for &str -> String; use free function for notify actions; use clone() where it's more obvious
and use explicit variants so we know we have to consider this match if we were to add a new variant later
* nit: rename RuleCommands::set_enabled to set_enabled_internal
* nit: test modules don't need to be publicized
* tidy tests
* nit: pass an owned RuleCommands in Rules::apply
* add comments/questions in Rules tests
* nit: no need to publicize the tests module
* tweak NotificationSettings tests too
* rustfmt
---------
Co-authored-by: Benjamin Bouvier <public@benj.me>
When the sync has been terminated and restarts (e.g. an app is going from the background to the foreground), then
the sliding sync is always restarting in growing mode, independently of the previous state of the sync (error or normal
termination). This is something the current sliding sync proxy can't handle nicely, because it prioritizes a change in
parameters over live data; as a matter of fact, to alleviate the burden of the proxy, we can try to only restart in
growing mode if we ran into an error (and not if we had a normal termination).
It ensures that we know exactly what's happening here. Tests are still
valid without any changes in the expected data, but it prevents a
potential breaking.
The Sliding Sync list `all_rooms` and `visible_rooms` of the `RoomList`
must have the exact same configurations for `sort`, `filters`, and
`bump_event_types`. Instead of maintaining the same code, this patch
adds a new function: `configure_all_or_visible_rooms_list` that
configures the lists exactly the same. This function also explicitely
configures `sort` so that we do no rely on the default values from
`SlidingSyncListBuilder`.
This patch configures the same `bump_event_types` for the
`visible_rooms` list than for the `all_rooms` list, so that we may be
sure that the sorting/ordering is the same between the two lists.
* ui: Move EventSendState into timeline::event_item::local module
* [WIP] ui: Make pending local echoes stick to the bottom of the timeline
* test: update more test expectations
* chore: tweak comment to slightly better reflect reality
* nit: remove else after return
* fix: the item's insert position is insert_idx, not `items.len()` anymore
* fix: look for remote echo before local echo when processing send state
Previous code assumed that the latest timeline items would be the most recent, and that
if there was a remote echo, it would always be after the local echo, because of that.
That's not the case anymore, so we must look for possibly a remote echo first, and then
if we find it, apply the late update process.
Also, there might remain a day divider added by the local echo, if it were inserted last.
I'm not sure it covers all the cases, but I've now made it so that the day divider is removed
if it was the last element.
* feat: switch strategy; keep on pushing if there's nothing in the timeline yet
* Revert "test: update more test expectations"
This reverts commit 400cc93ba7c98042a28b5e8d5042899e854f6cff.
* test: reset test expectations
* Address review comments
* fix: don't mix up latest event with any status, with latest non-failed event index
---------
Co-authored-by: Jonas Platte <jplatte@matrix.org>
For verification-by-emoji, the spec uses the word "Aeroplane" rather than
"Airplane". Element-web expects us to use the specced words (and will otherwise
complain about the lack of i18n data).
It seems like following the spec will help maintain consistency.
Common extensions are confusing, and they've included `e2ee` and `to-device` by default. This is not a sane default anymore,
now that there's the concept of `EncryptionSync`: it's either we have the encryption sync that enables e2ee and to-device +
a room list sync that doesn't, OR we have a single room list that has both.
Room List was misconfigured to always use `e2ee` and `to-device`, which was incorrect when it's running with the `EncryptionSync`
in the background. This is now removed, and properly tested.
This implements a new time lease based lock for the `CryptoStore`, that doesn't require explicit unlocking, so that's more robust in the context of #1928, where any process may die because the device is running out of battery, or unexpected flows cause a lock to not be released properly in one or the other process.
```
//! This is a per-process lock that may be used only for very specific use
//! cases, where multiple processes might concurrently write to the same
//! database at the same time; this would invalidate crypto store caches, so
//! that should be done mindfully. Such a lock can be acquired multiple times by
//! the same process, and it remains active as long as there's at least one user
//! in a given process.
//!
//! The lock is implemented using time-based leases to values inserted in a
//! crypto store. The store maintains the lock identifier (key), who's the
//! current holder (value), and an expiration timestamp on the side; see also
//! `CryptoStore::try_take_leased_lock` for more details.
//!
//! The lock is initially acquired for a certain period of time (namely, the
//! duration of a lease, aka `LEASE_DURATION_MS`), and then a "heartbeat" task
//! renews the lease to extend its duration, every so often (namely, every
//! `EXTEND_LEASE_EVERY_MS`). Since the tokio scheduler might be busy, the
//! extension request should happen way more frequently than the duration of a
//! lease, in case a deadline is missed. The current values have been chosen to
//! reflect that, with a ratio of 1:10 as of 2023-06-23.
//!
//! Releasing the lock happens naturally, by not renewing a lease. It happens
//! automatically after the duration of the last lease, at most.
```
---
* feat: implement a time lease based lock for the crypto store
* feat: switch the crypto-store lock a time-leased based one
* chore: fix CI, don't use unixepoch in sqlite and do time math in rust
* chore: dummy implementation in indexeddb, don't run lease locks tests there
* feat: in NSE, wait the duration of a lease if first attempt to unlock failed
* feat: immediately release the lock when there are no more holders
* chore: clippy
* chore: add comment about atomic sanity
* chore: increase sleeps in timeline queue tests?
* feat: lower lease and renew durations
* feat: keep track of the extend-lease task
* fix: increment num_holders when acquiring the lock for the first time
* chore: reduce indent + abort prev renew task on non-wasm + add logs
Calling `RoomListItem::full_room` creates its associated `Timeline`.
ElementX calls `full_room` to get the `is_direct`, `avatar_url` and
`canonical_alias` values for _all_ rooms in the room list. Instead of
doing so, let's add those methods directly in `RoomListItem` so that the
`Timeline` isn't created for _all_ rooms.
So. `SlidingSyncList::update_room_list()` does the following:
1. It adjust room list entries,
2. It updates the `maximum_number_of_rooms`,
3. It applies the sync operations on the `room_list` entries,
4. It updates the `room_list` entries for rooms outside the sync
operations.
SlidingSync answers with a `lists` and `rooms`. The `room_list` entries
is updated as follows: either a sync operation from `lists` has been
applied and the entries are updated, or `rooms` contains rooms that
are not updated by `lists` but that receive new events, and thus must
trigger an update in the `room_list` entries.
This patch changes a little bit the last part of this. Initially,
we were updating rooms that are part of `rooms` but absent of
`lists` sync operations, only **for the current list's ranges**.
But that's wrong! First, we have noticed a bug here: the
correct code wasn't `skip(start).take(end.saturating_add(1))` but
`skip(start).take(end.saturating_add(1) - start)`. Note a big deal, we
were iterating on more entries like it was necessary, but everything
was filtered later, so no bug, just useless computations. Second,
this `skip` and `take` is actually… useless. A room to which we have
subscribed can be out of any range, but we still want `room_list`
entries to receive an update for that particular room.
This patch fixes that once and for all.
In practise, the bug wasn't happening because if someone subscribes
to a room, its timeline is likely to be fetched, and updates will be
received, but still, this is better now from a SlidingSync strict point
of view.
This patch does the following:
1. changes `add_timeline_listener` to be async, thus removing one
`RUNTIME.block_on`.
2. simplifies the logic by adopting a more classical pattern we use
elsewhere:
* removes one `spawn_blocking`,
* let's move the `listener` into the task as a `Box` instead of
cloning it as an `Arc`.
Imagine the room list has the viewport set to the range `0..=19`. The
user scrolls quickly to `50..=69` to see something below and immediately
scrolls back to its initial position, without stopping the scroll
at any moment in between. The app using this API might update the
viewport from `0..=19` to… `0..=19`. Updating the viewport, updates the
`visible_rooms` sliding sync list. Since a list is modified, the current
sync-loop iteration is skipped over and a new one restarts, i.e. the
current in-flight request is cancelled… for nothing.
This patch prevents this situation. The current viewport ranges is
stored in `RoomListService`. `RoomListService::apply_input` used to
return `Result<(), Error>`, now it returns `Result<InputResult, Error>`.
The `InputResult` enum is a new type. It represents whether an input has
been applied or ignored, which must be differentiate from errors.
So now, if the viewport changes and it's not different from the previous
value, `InputResult::Ignored` is returned.
According to the spec, when a device receives a verification request and wants
to refuse it, it should send the `m.key.verification.cancel` to the originating
device, rather than broadcasting it.
This opens up a possible race condition where an embedder calls the method twice, generating two new rooms,
but then it's fine as long as they don't cache them somewhere, which we expect they don't.
That avoids recreating a timeline object every time a user calls `RoomListService::room()` with the same room id, so that should speed up
operations like fetching the latest event for rooms we've already entered before.
This patch changes the capacity of the internal buffer of
`ObservableVector` for `SlidingSyncList::room_list` from 16 to 4096.
With an increased capacity, we reduce the probability to send a
`VectorDiff::Reset` to subscribers. `Reset` are happening quite often
for apps using `matrix-sdk`, and it impacts their performance. This
patch tries to improve this situation. This `room-list` contains
`RoomListEntry`, which is can cheap in terms of memory, compared to the
impact of sending `Reset`s too often. That's a tradeoff.
* bindings: Ignore silently malformed userId in migration of tracked users
* Clean: collect will now never fail
Co-authored-by: Benjamin Bouvier <public@benj.me>
* crypto: implement more primitives for the MemoryStore to work in tests
* crypto: change the shape of the `CryptoStoreLock` API
In particular:
- make the lock work across multiple threads of the same process trying to acquire it,
using `num_holders`.
- add a mechanism to get the lock only once (for the NSE process, in case the main app
had acquired the lock before).
* client: add a cross-process crypto store lock, enable it with `Encryption::enable_cross_process_store_lock`
* client: make `preshare_room_key` a critical section of the cross-process lock
* sliding sync: make it possible to define different timeouts for a `SlidingSyncInstance`
This will be handy for the NSE process on iOS, which has very little time to wait for the proxy's responses.
* feat: implement the `EncryptionSync` API (renamed from `Notification` API)
* fixup! client: add a cross-process crypto store lock, enable it with `Encryption::enable_cross_process_store_lock`
* feat: allow disabling e2ee / to-device in the RoomList API
* feat: use same SS id for main/NSE process, reload to-device token from disk before each encryption sync
* fix: better error handling if restoring the to-device token failed
* feat: add logs for the locking functions
* test: add a few tests for encryption sync
* feat: add `reload_caches` method in the `EncryptionSync` + FFI bindings
* chore: clean up FFI loop
* encryption sync: Remove unused errors, specialize some errors
* feat: include termination reason in the encryption sync loop
* feat: add more logs
* chore: fmt + clippy + doc
* comment: precise only in the presence of another process
* Tweak `room_list` APIs to include `_with_encryption` variants
* chore: rustfmt
This patch moves `RoomListService::entries` and `::filtered_entries`
inside `RoomList`. It also implements `RoomList::new`. Finally,
it implements `RoomListService::all_rooms` and re-implement
`RoomListService::invites`.
Basically:
```rust
// 1.
room_list.entries().await?
// 2.
room_list.invites().await?
```
becomes:
```rust
// 1.
room_list.all_rooms().await?.entries()
// 2.
room_list.invites().await?.entries()
```
`all_rooms` and `invites` both return a `RoomList`.
This patch introduces `State::Error`, which replaces
`State::Termianted`. And a “new” `State::Terminated` state is added.
They do the same, but their semantics is different. `Error` is when an
error happened, and it's fine to restart the sync again. `Terminated`
is when a termination has been reached (either because SS sync-loop
has ended naturally, or because `RoomList::stop_sync` has been called),
in this case, it may not be fine to restart the sync automatically for
example.
This patch renames `State::FirstRooms` to `State::SettingUp`, and
`State::Running` as requested by users.
It hides the “inner Sliding Sync logic”, the Sliding Sync lists etc.
This patch implements `RoomList::stop_sync` on the FFI bindings.
Technically, it's no more useful to store the `TaskHandle` of the
`sync`, but it's still here, in case of.
This patch implements `RoomList::stop_sync`. The goal is twofold:
1. It forces to stop the syncing, thus putting the state-machine into
the `Terminated` state, which is semantically better than “stop
polling the `sync`'s `Stream`”.
2. It literally forces to stop the syncing. It cancels pending futures,
it cancels in-flight HTTP requests etc. It's a more robust way to
stop the `RoomList` sync.
It has an account field besides the issuer field.
Also store it as immutable, the data used for authentication will be
stored in another variable.
Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
`SlidingSync::sync` had a legacy behaviour when a `M_UNKNOWN_POS`
error was received from the server: It was resetting `pos` and sticky
parameters before re-running a sync-loop iteration, hoping to get a
valid response from the server after that. This retrying
mechanism was running up to 3 times in row (represented by the
`MAXIMUM_SLIDING_SYNC_SESSION_EXPIRATION` constant) before stopping the
`sync` for real.
While it seemed a good idea, it actually brings numerous problems:
1. Each iteration in the sync-loop generates a new request, thus making
the `ranges` of the requests to move forwards. For `SlidingSyncList`
that are in `Selective` sync-mode, there is no problem, but for
`Growing` or `Paging` sync-modes, the `ranges` increased. Thus,
when a `SlidingSync` session expires, instead of returning to
the caller to do something clever (like what `RoomList` does: start
again with a small range so that the “first” sync after a session
expiration is guaranteed to be fast), it was running larger and
larger requests, up to 3 times.
2. `M_UNKNOWN_POS` _is_ an error. Yes, `SlidingSync` must reset `pos`
and must invalidate sticky parameters, but the `sync` must be
stopped, and the error must be returned so that the caller can do
something about it. Until now, this error was likely to be missed by
the caller.
3. This legacy mechanism was forbidding `SlidingSync::sync`'s caller to
do something with this special error. And since the caller was blind to
this error, it disallowed more smart error management.
This patch removes this legacy retry mechanism entirely. When
`M_UNKNOWN_POS` is received from the server, `pos` is reset and sticky
parameters are invalidated as before, but the error is returned like any
other error.
This patch renames and updates an existing test that was testing sticky
parameters invalidation on `M_UNKNOWN_POS`, to include some assertions
on `pos` and to ensure that the sync-loop is stopped accordingly.
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 https://github.com/matrix-org/matrix-rust-sdk/issues/1961.
Usually it's impossible to create a Olm session from a pre-key message
twice. The one-time key that should be used for the 3DH step will be
used up and we're going to throw a `MissingOneTimeKey` error.
This used to be true and unproblematic until we added fallback keys,
these keys will not get discarded immediately after they have been used
once.
This means that a pre-key message, for which we already have a Session,
but decryption for it fails, might create a new Session overwriting the
existing one which will essentially reset the ratchet.
This implements a value-based lock in the crypto stores. The intent is to use that for multiple processes to be able to make writes into the store concurrently, while still cooperating on who does them. In particular, we need this for #1928, since we may have up to two different processes trying to write into the crypto store at the same time.
## New methods in the `CryptoStore` trait
The idea is to introduce two new methods touching **custom values** in the crypto store:
- one to atomically insert a value, only if it was missing (so, not following the semantics of `upsert` used in the `set_custom_value`)
- one to atomically remove a custom value
Those two operations match the semantics we want:
- take the lock only if it ain't taken already == insert an entry only if it was missing
- release the lock = remove the entry
By looking at the number of lines affected by the query, we can infer whether the insert/remove happened or not, that is, if we managed to take the lock or not.
## High-level APIs
I've also added an high-level API, `CryptoStoreLock`, that helps managing such a lock, and adds some niceties on top of that:
- exponential backoff to retry attempts at acquiring the lock, when it was already taken
- attempt to gracefully recover when the lock has been taken by an app that's been killed by the environment
- full configuration of the key / value / backoff parameters
While it'd be nice to have something like a `CryptoStoreLockGuard`, it's hard to implement without being racy, because of the `async` statements that would happen in the `Drop` method (and async drop isn't stable yet).
## Test program
There's also a test program in which I shamelessly show my rudimentary unix skills; I've put it in the `labs/` directory but this could as well be a large integration test. A parent program initially fills a custom crypto store, then creates a `pipe()` for 1-way communication with a child created with `fork()`; then the parent sends commands to the child. These commands consist in reading and writing into the crypto store, using a lock. And while the child attempts to perform these operations, the parent tries hard to get the lock at the same time. This helps figuring out a few issues and making sure that cross-process locking would work as intended.
This patch renames `SlidingSyncState` to `SlidingSyncListLoadingState`
because:
1. It's about a list information,
2. It's about the loading state, not a generic state.
The MSC3575 says that if no `ranges` for a list is provided, it defaults
to `0..=99`. We don't want that! This patch sets the default value to
`0..=19` for the `visible_rooms`.
* feat: lazily generate and include the transaction id, only if it's useful
* chore: add a small `LazyTransactionId` wrapper that ensures it's only created once
---------
Signed-off-by: Benjamin Bouvier <public@benj.me>
`RoomList::state` provides a `Subscriber` to the `State`. This patch
modifies the way the state is tested, to ensure that there is always
an update broadcasted even if the state is the same (e.g. if the state
moves from `CarryOn` to `CarryOn`).
This patch updates `RoomInner::timeline` and `::sneaky_timeline` to
`AsyncOnceCell<Arc<Timeline>>`. Adding `Arc` allows to copy the timeline
if necessary more easily.
This patch updates `Room::add_timeline_listener` to return a `RoomTimelineListenerResult`, a new type defined as:
```rust
pub RoomTimelineListenerResult {
items: Vec<Arc<TimelineItem>>,
items_stream: TaskHandle,
}
```
It is not possible to cancel the `items_stream` by cancelling the
`TaskHandle`. Without this, dropping `Room` won't drop the `Timeline`,
as a clone is moved inside the spawned task. As a consequence, it will
endlessly call the listener.
What the FFI user wants is to subscribe a listener to the
timeline updates. This feature is already supported by
`room_item.full_room().add_timeline_listener()`. So we can safely
remove `RoomListItem::timeline` as we are sure it's never going to be
used for now.
It's possible to do `room.inner_room().room_id()` but it's just simpler
to call `room.id()`. This patch adds this shortcut.
It's especially useful for FFI users, where creating a “full
room” (`room_item.full_room().room_id()`) may be expensive.
This patch adds the `subscribe` and `unsubscribe` method on `Room`.
To achieve that, `Room` receives an `Arc<SlidingSync>`, as the
subscription methods are on `SlidingSync`. It's just easier to put them
on `Room` for the API consumer. It makes the API also more elegant.
Finally, the patch adds the appropriate test.
Since stripped and non-stripped room infos use the same types,
the separation is not necessary anymore.
Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
… and simplify wasm spawn implementation.
This reduces the differences on wasm vs. non-wasm. Of course it's still
possible to rely on details of the different error types, but at least
both implement a few common traits:
`Debug`, `Display`, `Error`.
Look through m.room.member events when processing Sliding Sync
responses, finding those that refer to our user's membership, so that we
can recognise which rooms we have left.
Instead of storing `sliding_sync_proxy` inside the `Client`, this patch
updates the code to store it inside `matrix_sdk::Client` directly.
That way, there is unique place where to store the sliding sync proxy
URL.
This patch changes `matrix_sdk::Client::sliding_sync_proxy` to be a
`std::sync::RwLock<Option<Url>>` instead of `tokio::sync::RwLock<_>`.
It means that all methods reading or writing this field are sync instead
of async, which makes the code a lot more easier. Having an async-aware
lock wasn't necessary here.
* chore: add comments explaining what the position markers are
* feat(sdk): add scaffolding for sticky parameters
* test(sdk): add tests for the sticky parameters API
* feat(sdk): include the bump_event_types in the sticky parameters
* WIP: add TODO comments for deeply nested sticky parameters
* test(sdk): add extra test for sticky parameters
* feat: add extensions in the sticky parameters + test request generation + test since token
* chore: fix merge + get rid of unused inner extensions
* feat: use sticky parameters for lists too
* chore: Introduce the StickyManager, a reusable way to manage per-data sticky parameters
* chore: move the sticky scaffolding to its own module
* chore: clippy + fmt
* chore: get rid of `update_to_device_token`
* review: append SlidingSync prefix to Stick* data structures
* address review comments
* Replace nbsp with regular space
* Get rid of a write-access to a lock (because thanks inner mutability!)
* Add an integration test when losing `pos` in sticky parameters, and avoid `block_on`
Signed-off-by: Benjamin Bouvier <public@benj.me>
This patch implements `RoomList::rooms` to fetch one room. The type
name is `RoomListRoom` to avoid any collision with an existing `Room`
type already.
`RoomListRoom` implements `name`, `timeline` and `latest_event`.
By returning a `Subscriber`, one can call `Subscriber::get` to get the
inner value. Thus, having `state` and `state_stream` is useless. It's
possible to return have `state` which returns `Subscriber`, and we get
one value for two usages.
The example in MSC3575 shows a room with invite_state property, but also
timeline, joined_count etc. properties. That may or may not be very
likely in practice, but I think it makes sense to check for all these
properties even when the room is an invite, and use them if they are
present.
Added some comments based on my understanding of MSC3575. Previously we
had a FIXME about how we were setting the room state to invite or join
based on the presence or absence of the invite_state property. I think
this behaviour is correct, so I added some comments explaining why.
The functional change here is to note that we call
store.get_or_create_stripped_room to find/create the stripped room, and
this actually deletes any room of this ID that is in the store, so our
later call to store.get_room would always fall back to getting the
stripped room, which we already have, so there was no need to do the
get_room call at all.
This patch uses a new `async-once-cell` crate to make `Room::timeline`
and `Room::sneaky_timeline` by making them lazy. Their values are
computed lazily, on-demand, when calling the `Room::timeline` method or
`Room::latest_event`.
This extension was enabled by both ElementX apps by default, along with the e2ee / to-device extensions that will be handled
by the new Notification API. To maintain the current behavior, it's important to re-enable this extension before getting
RoomList in production.
Signed-off-by: Benjamin Bouvier <public@benj.me>
By default, get_media_file will attempt to use a default directory
for temporary files and directories. Unfortunately, there might not be
such a thing on some older Android devices, which use per-application
directories.
For this specific case, an optional temporary directory parameter
is added to the `get_media_files` parameters, so one can provide their own
temporary directory path.
This new parameter is expected to be set at least on Android; other platforms
should work just fine without it.
Signed-off-by: Benjamin Bouvier <public@benj.me>
This was a temporary replacement I've made, waiting for a PR of mine to be merged, and it's been merged since then. Oh well.
Signed-off-by: Benjamin Bouvier <public@benj.me>
* sliding sync: infer the storage key from the loop id and user id
* chore: rename `sync_id` to `id`
* chore: check that the sliding sync id is less than 16 chars
* chore: rejigger the storage key creation logic
Now the prefix is visible only in the `format_storage_key_prefix` function, and other `format_storage_key` function will be based off that.
* chore: update sliding sync README with API updates and fix outdated information
* chore: clippy + fix test
Signed-off-by: Benjamin Bouvier <public@benj.me>
This patch handles errors from the Sliding Sync loop properly.
When an error is received, the `RoomList` state machine enters the
`Terminated` state. Immediately after that, the sync stream is stopped.
When the sync stream is restarted, the next state will be calculated.
When the current state is `Terminated`, the next state is the state
that led to the `Terminated` state. To avoid having a “first” huge sync
(imagine a room list of 1000 rooms, you don't want to “resume” from an
error with a first sync for 1000 rooms), lists are partially “reset”
depending of the state where the machine is in.
An important test has been added to test all possibles cases with errors
and list' states.
First off, `SlidingSync::sync_once` no longer returns
`Option<UpdateSummary>` but `UpdateSummary`. It simplifies the rest of
the patch.
Next, `SlidingSync::sync` returns either `Ok(…)` and continues to sync,
or it returns `Err(…)` and it stops the sync stream immediately. No more
error could be returned with the stream to continue syncing.
Finally, the `UnknownPos` error no longer emits an err, but silently
skip over the sync-loop until the threshold is reached; which in this
case will generate a proper error.
This patch changes `RoomInner` to have an `room:
Option<matrix_sdk::room::Room>` field to `room: matrix_sdk::room::Room`.
`room` is not longer an `Option`.
Then, it's easier to have a new `timeline: Timeline` field, along with a
`sneaky_timeline: Timeline` field. Both are used by the new
`Room::timeline()` and `Room::latest_event()` method.
This patch implements `RoomList::room`, which returns a `Result<Room>`:
the room ay or may not exist.
A new `Room` type is created. It wraps an `Arc<RoomInner>` type, so that
it's cheap to clone it.
The `RoomInner` type owns a `SlidingSyncRoom` and
`matrix_sdk::room::Room` type. If some API or data are missing on
the former, the latter acts as a backup. This is the case for the
`Room::name` method which makes it best to return a name to the caller.
This patch lands the first design of the `Input` API. An `Input` is
something external that can be understood by the `RoomList` state
machine. The first inpuput is `Viewport` to change the “viewport” of the
`RoomList`, which translates to the change of the ranges of one specific
Sliding Sync list.
This patch implements `RoomList::update_entries_stream_filter` which
allows to… change the… entries stream filter. This patch also provides a
test for that. How nice it is.
This patch adds a new method on `SlidingSyncList` named
`room_list_filtered_stream`, which uses the new `eyeball-im-util` crate
with its new `FilteredSubscriber` type.
This patch does several things.
First, `SlidingSync::on_list` is now async, and accept async closures.
Second, `SlidingSync::lists` and `::rooms` are behind an `AsyncRwLock`
instead of a `StdRwLock`. The rest of the patch updates the consequence
of this.
This patch replaces a panic (`Option::expect`) by a `tracing::error`
log.
Imagine the Sliding Sync proxy responds with the following payload:
```json
{
"pos": …,
"lists": {
…: {
"count": …,
"ops": [
{
"op": "INSERT",
"index": 0,
"room_id": !foo:bar.org",
}
]
}
},
"rooms": []
}
```
It's an invalid response, as it will update the room list entry (because
it's present in `lists.$list.ops`), but the room won't be created (it's
absent in the `rooms`).
This situation creates a panic in the patched code. We don't want to
crash if the server replies with invalid data.
Ultimately, we should check that the sync operations are valid regarding
the rooms' updates.
This patch uses `FutureExt::now_or_never` so that we don't wait on the
future to be resolved to get a result. If we have a bug in our code and
the test expects a value, the test will hang forever, which is not a
desired behavior. With `now_or_never`, this situation cannot happen.
The only reason why a sender can fail to send a message is because there
is no receiver. In the current design of `SlidingSync`, there is only
one receiver active per sync-loop. Thus, calling `SlidingSync::add_list`
may fail if `sync` has not been started. Hence the need for a new
`internal_channel_send_if_possible` method which will never fail: it
will send a message is possible, otherwise it won't do anything.
This turns more functions infallible.
`tokio::sync::broadcast` is interesting as it's possible to
generate receivers per subscribers. Being able to do that allows
us to remove the new for an `AsyncLock` around the receiver in
`SlidingSync::internal_channel`, and it can even remove the need to hold
a receiver entirely!
Another improvement is that new receivers can't receive past messages,
so we no longer need to drain the internal channel.
Another improvement is that the sender' `send` method is synchronous!
Which helps to make many functions no longer `async`.
Something weird is happening with ElementX iOS. When
`SlidingSync::stop_sync` is called, the internal message `SyncLoopStop`
has time to be sent in the internal channel, but iOS decides to suspend
the code before the sync-loop processes it. When ElementX decides to
start the sync-loop again, it immediately processes the `SyncLoopStop`
message, and… stops the sync-loop.
This patch ensures that `SlidingSync::sync` drains the internal channel
before starting the sync-loop for real. `tokio::sync::mpsc::Receiver`
type has no `drain` method, so this patch implements its
own logic by calling `try_recv` in a loop, until it returns
`Err(TryRecvError::Empty)`.
* feat: introduce SlidingSyncSelectiveModeBuilder
* feat: get rid of `CannotModifyRanges` \o/
* chore: rustfmt
* chore: remove scope in test
* chore: move request generator update to its own mod, gets rid of `set_ranges`
* chore: add comments on the request generator methods
* chore: sink one range_end definition into the match arm that matters
* ffi: remove unused `add_range` method
* test: update incorrect test expectation
* chore: make clippy happy
* address first review comments
* review: don't reuse the ranges field for two things
* chore: use a builder pattern for sliding sync selective mode
* address review comments + CI
Signed-off-by: Benjamin Bouvier <public@benj.me>
In case it's not obvious to drop the `Stream` returned by
`SlidingSync::sync` immediately to “stop” the sync-loop, one can use the
new `stop_sync` method to do achieve the same result.
The `VerificationRequest` object is used to control the flow of the
verification but only up to a certain point.
Once we start handling of different specific verification flows (i.e.
SAS or QR code verification) the `VerificationRequest` object creates a
child object of the Verification type.
This patch adds a new `VerificationRequestState` variant called
`Transitioned` which holds the child verification object as associated
data.
This makes it much simpler to go through the whole verification flow by
allowing users to just listen to the `VerificationRequest::changes()`
method.
We suspect that there might be too many internal messages being pushed, causing a deadlock on the senders' side.
This attempts to increase the value of the buffer to give it more leeway.
Signed-off-by: Benjamin Bouvier <public@benj.me>
The `SlidingSyncListInner::timeline_limit` and `::ranges` fields were
observable (behind `eyeball::Observable`). It was actually useless
as those fields were never exposed to the public API, thus it was
impossible to subscribe to them.
This patch cleans up that. `timeline_limit` and `ranges` are no longer
observable.
The comment and the check in the code were contradicting each other; the comment was wrong, and the code was right, so there's that.
Signed-off-by: Benjamin Bouvier <public@benj.me>
This patch removes our dependency to libolm completely. This should
allow WASM targets to use the backups_v1 feature of the
matrix-sdk-crypto crate as well.
First off, this patch changes `pushd(…)` to `pushd(…)?` so that errors
are propagated.
Second, instead of assuming that all crates live in `crates/`, let's
allow to precise a prefix, like `crates/` or `bindings/` directly in the
“folder” path of `args`.
Changing the sync-mode of a list on-the-fly is necessary to optimise the
new `RoomList` API. For example, we can start with a list in a selective
mode, a range of `0..=50` and a `timeline_limit=0` to fetch the
beginning of the room list, and then _change_ the sync-mode to growing
to continue to sync the room list in the background.
Today, this is done with 2 lists and a merge of the lists, but
(i) this is error-prone, (ii) this is not optimal. Thank to
`SlidingSyncList::set_sync_mode`, merging lists is no more necessary,
thus removing a class of bugs in client's code.
This patch replaces `sync_mode` on `SlidingSyncListBuilder` by
`sync_mode_selective`, `sync_mode_paging` and `sync_mode_growing`, which
removes the need to use `SlidingSyncMode` directly.
This patch also removes `batch_size`, `room_limit` and `no_room_limit`
as it's now arguments of `sync_mode_paging` and `sync_mode_growing`.
`SlidingSyncMode` was previously declared as:
```rust
enum SlidingSyncMode {
Selective,
Paging,
Growing,
}
```
Now, the new declaration is:
```rust
enum SlidingSyncMode {
Selective,
Paging {
batch_size: u32,
maximum_number_of_rooms_to_fetch: Option<u32>,
},
Growing {
batch_size: u32,
maximum_number_of_rooms_to_fetch: Option<u32>,
}
}
```
First off, it helps to remove the `full_sync_batch_size` and
`full_sync_maximum_number_of_rooms_to_fetch` methods and fields from
`SlidingSyncListBuilder`. It was containing default values in case of.
That was useless and needed to be fixed. Also, calling a `full_sync_*`
method with the `Selective` mode had no effect, which could disturb
the user. Well, now everything is clean on that front.
Second, `SlidingSyncListRequestGenerator` no longer has constructors,
but a `From<SlidingSyncMode>` implementation. However, the constructors
now live in `SlidingSyncMode` with `new_selective`, `new_paging` and
`new_growing` methods which are helpers. All in all, it makes creating a
`SlidingSyncListRequestGeneator` simpler: just call `sync_mode.into()`
and boom.
Finally, this patch removes the `default_with_fullsync` list builder
helper, which makes no sense at all.
`SlidingSync::subscribe`, `::unsubscribe` and `::add_list` are
now async. `subscribe` has been renamed to `subscribe_to_room` and
`unsubscribe` to `unsubscribe_from_room`.
`SlidingSyncRoom::subscribe_and_add_timeline_listener` has
been removed, and replaced by a new `::subscribe_to_room` and
`::unsubscribe_from_room` methods (the `::add_timeline_listener` method
already exists).
`TaskHandle::finalizer` is no longer necessary, thus this code has been
cleaned up.
This bug has been found by @bnjbvr, all the credits go to him. I've just
added some comments around his code.
Prior to this patch, the room unsubscription buffer
(`SlidingSync::room_unsubscriptions`) was reset before the request was
sent. So if something went wrong, the next request would not include the
room unsubscriptions.
This patch updates this behavior. First, it replaces `Vec` by `HashSet`
to avoid a O(n^2) look up.
Second, a copy of room unsubscriptions used by the request is kept, so
that it can be used to cherry-pick which room unsubscription to remove
from the buffer once a response from the server is received. It's
important to not clear the entire room unsubscriptions buffer as more
unsubscriptions could have been inserted meanwhile.
This has slightly drifted from the initial design I thought about in the issue.
Instead of having `build()` be fallible and mutably borrow some substate (namely `BTreeMap<OwnedRoomId, SlidingSyncRoom>`) from `SlidingSync` (that may or may not already exist), I've introduced a new `add_cached_list` method on `SlidingSync` and `SlidingSyncBuilder`. This method hides all the underlying machinery, and injects the room data read from the list cache into the sliding sync room map.
In particular, with these changes:
- any list added with `add_list` **won't** be loaded from/written to the cache storage,
- any list added with `add_cached_list` will be cached, and an attempt to reload it from the cache will be done during this call (hence `async` + `Result`).
- `SlidingSyncBuilder::build()` now only fetches the `SlidingSync` data from the cache (assuming the storage key has been defined), not that of the lists anymore.
Fixes#1737.
Signed-off-by: Benjamin Bouvier <public@benj.me>
Co-authored-by: Jonas Platte <jplatte+git@posteo.de>
Currently, the `body` of a `SignatureUploadRequest` includes a spurious
`signed_keys: {...}` property in which the actual content is wrapped. Fix that.
We were using `txn_id` as a way to identify the running stream. Now
things are cleaner so we can remove this “debug tool”. This class of
problems should not appear anymore. For the record, the biggest problem
was the following: It was possible to start multiple `stream`s at the
same time, and thus a stream could receive a response sent by another
stream. Since we no longer need to restart SlidingSync anymore (i.e. no
need to start multiple `stream`s at the same time), this problem should
not happen.
Prior to this patch, the `v4::Response`, aka the SlidingSync response,
was processed by the client and transformed into a `SyncResponse` into
`sync_once` before being passed to `handle_response`. Now it's done in
`handle_response`.
This is not mandatory _now_, but it will be helpful in a close future.
* chore: use RangeInclusive instead of raw start/end integers for ranges in sliding sync
* chore: have timeline_limit use a u32 instead of a Ruma UInt
* chore: Remove all the `Into<u32>` generics on timeline_limit and ranges APIs
* chore: introduce a `Bound` type alias for u32
Signed-off-by: Benjamin Bouvier <public@benj.me>
This patch changes `SlidingSyncRoom` to move all its fields inside an
inner type `SlidingSyncRoom` behind an `Arc`, so that cloning is cheap,
and all clones are sharing the same state.
A `SlidingSyncList` has a state, which can be either `NotLoaded`,
`Preloaded`, `PartiallyLoaded`, or `FullyLoaded`.
A `SlidingSyncList` can be reset, either manually when
`SlidingSyncList::reset` is called, or when a range is updated for
example.
Resetting a list is modifying its state. Prior to this patch, the state
was set to `NotLoaded`. However, it's not entirely true. This patch
updates this behavior to the following rules:
* When the state is `NotLoaded`, it's kept at this state as nothing has
happened yet,
* When the state is `Preloaded`, the list is restored from the cache,
but nothing has happened yet too, so it's kept at this state,
* When the state is `PartiallyLoaded` or `FullyLoaded`, it means some
(or all) updates have been done, so the new state is `PartiallyLoaded`
after the reset.
The list' state is used mostly by the client to know whether a loader
should be prompted to the users. The ranges are modified when the
users scroll inside the room list for example: scrolling in the room
list doesn't imply the state should go to `NotLoaded`. Some data
have potentially be loaded, so changing the ranges should result in a
`PartiallyLoaded` state. It seems more logical once explained like this.
`invite_state` is managed when the Sliding Sync response is processed/
handled by the `Client`. Inside `SlidingSyncRoom`, it's not necessary to
maintain it.
This patch creates a constant to represent the
maximum number of timeline event to put in the cache:
`NUMBER_OF_TIMELINE_EVENTS_TO_KEEP_FOR_THE_CACHE`. Verbose, but easily
understandable.
This patch also renames a few variables.
This patch simplifies `SlidingSyncRoom::timeline_queue` from
```rust
Arc<RwLock<ObservableVector<SyncTimelineEvent>>>
```
to
```rust
Vector<SyncTimelineEvent>
```
First, we don't need to be observable. It's never observed since
it's private, and even privately, it's never observed, there is no
subscriber.
Second, no lock is required as updates happen synchronously.
Third, `Arc` is not necessary. We want each clone of `SlidingSyncRoom`
to not share any state across them.
Finally, this patch simplifies the iterator + `.push_back` by a simple
`.extend` to update the `timeline_queue`. Behind the scene, `impl Extend
for Vector` actually does an iterate + `.push_back`; let's keep our code
simple though.
`SlidingSyncRoom::is_cold` is not well-named. This patch introduces an
enum, named `SlidingSyncRoomState` which contains more detailed state:
`NotLoaded`, `Preloaded`, and `Loaded`.
The use of `AtomicBool` is also removed. Thus, we no longer need an
`Arc` for the state, which makes `Clone` more obvious: the state is not
shared across clones anymore.
A `SlidingSyncRoom` receives an Ruma
`api::client::sync::sync_events::v4:SlidingSyncRoom`. This value is
stored in the `SlidingSyncRoom::inner` field. From here, some getters
like `name()`, `is_dm()` etc. are using the `inner` field to compute a
result. There was one exception though: `prev_batch`. This value is part
of `v4::SlidingSyncRoom` but it was copied and updated in its own field:
`SlidingSyncRoom::prev_batch`.
I was wondering why. Turns out, there is no reason. Its getter
`prev_batch()` is public, but it's not used by the FFI bindings, so
basically nobody uses it (as this project is experimental as the time of
writing, we know our users).
This patch removes the `SlidingSyncRoom::prev_batch` field.
This patch also removes the `SlidingSyncRoom::prev_batch()` getter.
This patch finally removes the `FrozenSlidingSyncRoom::prev_batch` field
too.
Prior to this patch, to check a room exists, it was removed from the
collection, then re-added if it exists. Instead of doing this `remove`
+ `insert` dance, we can simply use `get_mut`, which also returns an
`Option`. This patch does that, in addition to rewrite the code to use a
`match` instead of a `if` + `else`.
`SlidingSync::subscribe` and `SlidingSync::unsubscribe` will cancel in-
flight requests, i.e. the `SlidingSyncInternalMessage::ContinueSyncLoop`
will be sent in the internal channel, just like what `SlidingSyncList`s
already do when a parameter is changed.
If the redaction already happened server-side, it is a bug for the
server to even still have access to the non-redacted form.
We don't accept the server data in this case, and also log a warning.
`on_built` is a method that registers a closure. This closure is called
when a list is built by `SlidingSyncListBuilder::build`. It receives
a `SlidingSyncList` and returns a `SlidingSyncList`, the list can be
updated or returned as is. It allows to configure a `SlidingSyncList`
right after it's built. `SlidingSyncBuilder::build` is responsible to
finalize the configuration, and to build all the lists. Once they are
built, the state is restored from the cache. If one wants to configure
a list before the state is restored from the cache, `once_built` will
serve well.
The timestamp value of the Notification was not reliable since it was the
timestamp in which it was generated internally, not the timestamp of when the
event was sent, now we instead expose the `origin_server_ts` of the
TimelineEvent.
`is_read` is also very unreliable, so it's just removed for now.
Currently this takes a `CrossSigningKeyExport`, but that's not a class you can
construct from the JS side. Instead, let's just pass in the individual keys.
Using the #[derive(uniffi::Object)] on certain types make them not show up
in the .aar file when building for Android. Could not determine why that is,
but removing the derive macro, and adding the empty interface declarations
inside the UDL, makes them show up.
We can still use #[uniffi::export] and #[uniffi::constructor].
Signed-off-by: Simon Farre <simon.farre.cx@gmail.com>
Currently, we can't pass the response to a `SigningKeysUploadRequest` into
`mark_request_sent`, which apparently we should be able to do.
So, we need to make `SigningKeysUploadRequest` have a `type` like the other
`OutgoingRequest`s, and add support for `SigningKeysUploadResponse` to
`OwnedResponse`.
Because since SlidingSync no longer restarts, the behaviour is really
different. The current way the tests are written cannot assert the full
behaviour. We need to rewrite this test suite entirely.
The unknown filter can be difficult to match with MembershipState,
depending on the store implementation.
Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
Storing as bitset is not future-proof.
Just fix the latest migration. We assume no-one used it yet.
Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
The MPSC channel that has been introduced in recent commits is now used
in this patch. The `SlidingSync::stream` method can be controlled via
the `internal_channel`.
This patch updates `SlidingSync::stream` to use the `tokio::select!
` macro to select any future that resolves first between the
`internal_channel` receiver, or `SlidingSync::sync_once`. Fairness is
biaised as the `internal_channel` has the priority.
This mechanism is already used by this patch: `SlidingSyncList::reset`
will send the `SlidingSyncInternalMessage::ContinueSyncLoop` to
“continue”… well… the sync loop, i.e. it will cancel in-flight waiting
for a response.
This entire mechanism removes the need to “stop” and “start”, i.e.
“restart” the `SlidingSync::stream` method manually, which was the
source of many bugs. Now everything is controlled internally.
Prior to this patch, `SlidingSync::add_list` was taking a
`SlidingSyncList`. However, we need to inject more data when building
the list, so let's modify `add_list` to take a `SlidingSyncListBuilder`
instead. It's even better for the user as calling `build()` isn't
necessary anymore.
It's not possible to clone a `SlidingSyncList` anymore. Why? Because
it's not correct. Prior to this patch, it was possible to add a list
to a `SlidingSync` instance, then add a clone of the same list to
another `SlidingSync` instance. Weird behaviors could happen, but more
importantly, for the next Sliding Sync design we are working on, it
could lead to gigantic bugs.
Removing `Clone` from `SlidingSyncList` makes the code simpler.
For example, `SlidingSyncList.inner` no longer needs an `Arc`, or
`SlidingSync::stream` no longer needs to clone all the lists.
The `SlidingSyncBuilder` now has a new method: `bump_event_types`, to
configure the `bump_event_types` HTTP request parameter. This value
is passed to `SlidingSync`, which is used to build the `SlidingSync`
request.
The format of the outbound group session struct has changed. We nowadays correctly rotate the group session if we can't restore it, but it's still good to avoid logging the error in this case.
An error is currently thrown if loading an outbound group session fails.
This error may only affect loading outbound group sessions, while other
store operations continue to work fine. As a result, the user is unable
to send messages, but can still use the application without any problems.
Since outbound group sessions are rotated frequently, we can simply
rotate the session if loading fails. However, if the error is related to
a more serious storage issue, persisting the newly rotated outbound group
session will also fail. If the error is specific to outbound group
sessions, the user will be able to continue using the application without
interruption.
The SQLite crypto store uses rmp_serde to serialize all the data we're
going to store. This works nicely for most things, one exception to this
is the OutboundGroupSession type.
The OutboundGroupSession type stores to-device requests to ensure that
the session doesn't get used before it is shared with the whole group
and to ensure that the to-device requests get restored if the
session gets restored after an application restart.
The to-device requests type critically contain `Raw<AnyToDeviceEvent>`,
the `Raw` type here being the serde_json::Raw type. rmp_serde seems to
serialize this just fine, but later deserialization fails.
We're avoiding the issue by using serde_json to serialize the
OutboundGroupSession.
Those methods are public, but never used by our current users.
Moreover, there is some big overlaps. For example, `storage_key`,
`cold_cache` and `no_cache` do the same thing: They update the
`storage_key` field. Or `add_fullsync_list` is just a helper that is
never used except in the tests etc.
The test does the following:
1. Create 2 (identical) lists,
2. Do a sync.
3. Assert that the 1st and 2nd lists are receiving an update,
4. Add a 3rd (identical) list,
5. Do a new sync,
6. Assert that 3rd list is receiving an update.
This last step is wrong. All lists should receive an update as they
are identical.
This patch updates a test to ensure that `room_list` receives “diff”
for rooms that are modified by a sync operations, but also by another
updates (like a new event).
The `SlidingSyncList.room_list` field is used to store the list of rooms
for a particular Sliding Sync list. It's used by the user to receive
“diff”s when a room sees its position modified. For example when a new
room receives a message, it “climbs back up” the entire room list to
be at the top place. Another example is when a new room is created,
some rooms will move around to give the new room a space. All those
moves will create “diff”.
However, the user also expects to receive a “diff” when a room has
received some updates, even if it's position doesn't change. For
example, when a room is already at the top of the list but receives a
new message: It has received an update, but its position stays the same.
This specific latter feature was implemented before, but it has been
removed by accident in https://github.com/matrix-org/matrix-rust-sdk/
pull/1699 (more specifically in https://github.com/matrix-org/matrix-
rust-sdk/pull/1699/commits/861a05be69a566d9a4ad125dc6ecb418d2b3210f).
At that time, it was not clear why the code was filtering for specific
filled room entires, to set the filled room entries, at the same
position. Zero comment, zero test. I didn't consider this as a feature
but as a bug.
So this patch re-introduces this feature. Hopefully in a more optimal
way. If a room has already triggered a first “diff” because of a
position change, it won't trigger a second “diff” because it has
received an update (which was the case before).
The `SlidingSyncList::handle_response` method has also been renamed
`update`, as it does update, whenever it comes from.
The code has been commented and documented to explain this feature.
Existing tests have been updated, especially for `apply_sync_operations`
which now ensure the `rooms_that_have_received_an_update` collections is
updated accordingly. Another test has been updated specifically to test
the “diff”s received by `room_list`.
This patch also re-uses the `format_storage_key_*` functions to use the
same key as the `restore_sliding_sync_state` function. It fixes a bug
where the keys were mismatching.
This PR is self explanatory.
Exposes said function to the FFI. Defined in the .adl file,
and defined to throw `ClientError`
Signed-off-by: Simon Farre <simon.farre.cx@gmail.com>
Co-authored-by: Damir Jelić <poljar@termina.org.uk>
In some cases, restoring client state from backups may cause Olm sessions
to become corrupted, resulting in a backward ratchet. As a consequence,
the receiving side is unable to decrypt messages. To address this issue,
the failed side initiates a new Olm session and sends a dummy encrypted
message.
However, this dummy message also creates a new Olm session on the
sender's side. To ensure that both sides use the same new session, we
must sort sessions by their creation timestamp before encrypting new
messages.
Although the session list was sorted correctly previously, we selected
the wrong side of the list, resulting in an outdated session being
selected instead of the newest one. This patch resolves the issue by
selecting the newest Olm session and adds a test to the session getter
logic to prevent reintroduction of this bug.
Prior to this patch, when `SlidingSync` was built with a storage key,
the storage was read to find a previous `SlidingSync` version that
was in a cache. To put a `SlidingSync` instance in the cache, it
was serialized to JSON. When one reads from the cache, the value is
deserialized. A problem happens when the internal representation of
`SlidingSync` and other types (e.g. `SlidingSyncList`) have changed (due
to an SDK update for example): Loading a previous state from the cache
results in an error.
After discussion with the teams, clients don't want to deal with
obsolete cache values. In such a scenario, (i) they cannot interact
with the cache to remove the obsolete entry, and (ii) their only
possibility is to log out and log in the user again. What an unfriendly
user experience!
This patch modifies this behaviour. When loading a `SlidingSync` type or
a `SlidingSyncList` type from the cache, 3 cases are now handled:
1. The cache entry exists and has been successfully deserialized: in
this case, we do our business.
2. The cache entry exists, but it wasn't possible to deserialize it: the
cache entry is declared as obsolete, and the entire `SlidingSync`
cache is cleaned up, so all entries for `SlidingSync` and
`SlidingSyncList` are removed from the storage,
3. The cache entry doesn't exist: we do nothing particular.
Note that if one cache is obsolete, all cache entries are removed.
The `SlidingSync::stream` method no longer resets the lists everytime
it is called. If one wants to reset the lists, they need to call
`SlidingSync::reset_lists`.
`SyncOp::Invalidate` means _invalidating_ a particular range. When a
room is `Filled`, it becomes `Invalidated`, when it is `Invalidated` it
stays `Invalidated`, and when it is `Empty` it stays `Empty`.
Before this patch, `Empty` was becoming `Invalidated`, which apparently
is a bug.
This patch also fixes out-of-bound accesses, and adds many tests.
Finally, this patch renames `update_state` to `update_room_lists`.
`SyncOp::Insert` means _inserting_ a new room ID. The `rooms_list`
contains all possible rooms (based on `maximum_number_of_rooms`, a value
returned by the server). Here, inserting = setting
`RoomListEntry::Filled` at a particular index of `rooms_list`, that's
it.
The previous code was doing very complex stuff, like removing things
around the `index` if something etc. It was using the requested ranges
(the range passed to the request) etc.
Applying `SyncOp` should be simple and is focused on updating
`rooms_list` only: the requested ranges have nothing to do here.
This patch also prevents against out-of-bounds acccesses, which wasn't
the case before.
This patch prevents out of bounds acceses for `SyncOp::Delete`, and adds
more tests.
This patch also removes a cast from `UInt` to `u32` to `usize`. It's now
from `UInt` to `usize` directly.
First off, this patch renames `ops` to `sync_operations` and `room_ops`
to `apply_sync_operations`.
Second, the `SlidingOp::Sync` was creating an out-of-bounds access
depending of the range present in the server's response. For example, if
the `rooms_list` contains 5 elements (because the
`maximum_number_of_rooms` is set to 5), and the server replies with:
```json
{
"op": "SYNC",
"ranges": [3, 17],
"room_ids": […]
}
```
the previous code was setting a new `RoomListEntry` at indices `3..=17`,
whilst the `rooms_list` contains only indices from `0..=4`. That's
annoying.
The previous code was also counting the number of `room_ids` for
nothing, just to execute the iterator that was applying the actual
changes in a `map`. Well, everything was fishy.
This patch updates the code to protect against an unexpected server's
reply by raising an `Err`. This patch also adds tests.
The `updated_rooms` argument was passed to `find_rooms_in_list` to
update the `room_list`: the update is setting the filtered room list
entry to `RoomListEntry::Filled`.
_But_, `find_rooms_in_list` was already filtering rooms which are
`Filled`. So it does… nothing: it filters rooms which are `Filled` to
update them to `Filled`.
So we can remove `find_rooms_in_list` because it becomes useless. And we
can remove `updated_rooms` too.
The `rooms_list` is updated by `rooms_ops` itself. Let's keep
modifications in one unique place.
The `update_state` method of `SlidingSyncListInner` has basically
2 cases:
1. For an initial response,
2. For other responses.
The code between the 2 cases were almost identical. Or, they could be
identical. The few exceptions are:
* In the first case, the `rooms_list` updates were taking the
form of a `VectorDiff::Append`, while the second case, it was a
`VectorDiff::PushBack`.
* In the first case, the `is_cold` flag was set to `false`.
It's fine for the clients to receive only a `VectorDiff::Append` event
only. So let's make it uniform.
And it appears that the `is_cold` field is now private, and never read
anywhere else. So… it's… basically useless. We can remove it! It was
previously used here to know which flow to use, but since we can make
both flows identical, its role becomes insignificant.
`create_range` and `SlidingSyncList::next_request` return a `Result`
instead of an `Option`. It was a legacy from the old `impl Iterator` of
`SlidingSyncListRequestGenerator`. But actually, when `create_range`
fails, it must return a `Err` value, not a `None` value.
And since it's a regular error, `sync_once` can propagate the
error. Thus, it is no longer necessary to “update” the list of
`SlidingSyncList`. It's not necessary to remove some items in this list
when the `next_request` method can no return a value: it always returns
a value, except when a error happens.
First off, `set_ranges`, `set_range`, `add_range` and `reset_ranges`
take a `&[(U, U)]` instead of a `Vec<(U, U)>` when a vector was needed,
or takes a `(U, U)` instead of 2 arguments when it was needed.
Second, all those methods now return a `Result<(), Error>`. The
`Error::CannotModifyRanges` is raised if the chosen `SlidingSyncMode`
doesn't allow to modify ranges. Basically, `Selective` does allow a
user to modufy the ranges, but there is no real ranges with `Growing` or
`Paging`. Let's make it an explicit error.
Finally, `SlidingSyncListInner` has 2 methods: `set_ranges` and
`add_range`, but without any “ranges check”; `SlidingSyncList` does call
the inner methods, but does the checks.
The `SlidingSyncListInner::find_rooms_in_list` method can be greatly
improved by using the `Iterator` API.
It was already using `Iterator::skip`, great! Let's continue by using
`Iterator::take` instead of using a stop index.
And let's use `Iterator::enumerate` to avoid using a mutable index.
`SlidingSyncList::find_room_in_list` and
`SlidingSyncList::find_rooms_in_list` aren't useful (and never used) by
the public consumer of this API. Let's remove it. They are only used for
internal purposes.
Before, the `update_request_generator_state` method was emitting an
`error!` log, but not an error. But that's clearly an error!
This patch updates the method to return a `Result<(), Error>`, and adds
a new variant to `Error`.
There are problems with `SlidingSyncListRequestGenerator`:
* It's part of the module API,
* It contains a clone of `SlidingSyncList`.
To create a `SlidingSyncListRequestGenerator`, one has to
call `SlidingSyncList::request_generator`. It was done in
`SlidingSync::stream`. The problem is that it clones `SlidingSyncList`.
So theoritically it is possible to create multiple request generators
for the _same_ list, and use them to send many requests and to update
the _same_ list with multiple responses. This is utterly error-prone and
can lead to really complex bugs to discover.
Moreover, it's a lot of clones. Cloning a
`SlidingSyncListRequestGenerator` isn't cheap as it means cloning a
`SlidingSyncList`. Moreover, cloning a `SlidingSyncList` isn't cheap as
it means cloning the entire struct.
Having `SlidingSyncListRequestGenerator` inside the module API also
makes the code of `SlidingSync` more complex (why having to deal with
lists and request generators at the same time? we must be very careful
to maintain both side by side? what if a list is removed but not its
request generator? and so on).
So. This patch simplifies all that.
First off, it extracts all the fields of `SlidingSyncList` into a
`SlidingSyncListInner` struct. Then, `SlidingSyncList` has only one
field: `Arc<SlidingSyncListInner>`. Boom, it's now cheap to clone it.
Second, `SlidingSyncListRequestGenerator` is only a
struct with constructors, but there is no extra methods.
`SlidingSyncListRequestGenerator` _no longer_ contains a
`SlidingSyncList`, and doesn't no need a list at all to work. It's just
values.
Third, `SlidingSyncList` holds a `SlidingSyncListRequestGenerator`, and
only one.
Fourth, `SlidingSyncList` (and `SlidingSyncListInner`) now has methods
to handle the internal request generator. The initial `impl Iterator for
SlidingSyncListRequestGenerator` becomes a simple
`SlidingSyncList::next_request` method.
Fifth, previously, the `SlidingSyncList::handle_response`
was never called directly by `SlidingSync`. It was called by
`SlidingSyncListRequestGenerator`! How confusing! `SlidingSync` called
`SlidingSyncListRequestGenerator::handle_response` which was calling
`SlidingSyncList::handle_response`. Now, the flow is more natural:
`SlidingSync` calls `SlidingSyncList::handle_response` and that's it.
Sixth, the `SlidingSyncList::handle_response` is now composed of 2
parts: updating the list itself, and updating the request generator. It
was kind of the case before, but onto two different types.
It was unclear which types were updating `SlidingSyncList`.
For example, `SlidingSyncList::state` was updated by…
`SlidingSyncListRequestGenerator`. Now `SlidingSyncList` is responsible
to update itself, and no one else.
Finally, `SlidingSync` no longer have to deal with
`SlidingSyncListRequestGenerator`. All it has is a set of
`SlidingSyncList`, and that's it!
The tests are still passing, hurray.
In `SlidingSyncListBuilder`, the `ranges`, `set_range`, and `add_range`
methods do not take a `u32` but a `U: Into<UInt>`. That's great!
However, in `SlidingSyncList`, the same methods take a `u32`. It makes
the API inconsistent.
This patch fixes that.
This patch renames `SlidingSyncList.set_ranges` to `ranges` so that
it matches the same terminology of the `SlidingSyncListBuilder` with
`ranges`, `set_range`, `add_range` and `reset_ranges`.
Consistency is important here.
The `SlidingSyncList::new_builder` method creates a new builder from a
`SlidingSyncList`. This method was missing some configurations, like
`full_sync_maximum_numbre_of_rooms_to_fetch`, `send_updates_for_items`,
`filters`, `ranges` and `timeline_limit`.
This patchs adds those missing configurations.
The `SlidingSyncListBuilder` struct has a `state` and a `rooms_list`
fields. Those fields are never updated and no setters or getters exist
to use them. There are just set with defaults, and passed to the final
`SlidingSyncList` type within the `build` method.
If a field is present in a builder type, it means they can be modified,
but it's not the case. So this patch removes them.
Event deduplication is supposed to be handled by the `Timeline`. Sliding
Sync is not responsible to de-duplicate the events, in our model. Having
two different de-duplication logics (one in Sliding Sync, and one in the
Timeline) could create weird situations, and hard to debug issues.
We give it a try by removing the deduplication from Sliding Sync.
The `m.no_olm` withheld code is supposed to be sent out only once for a
given device.
This patch prevents double sending of m.no_olm withheld code by adding
a test that ensures only one group session will send it out.
Previously, the logic for sharing group sessions was mostly correct, but
an edge case was forgotten where two group sessions attempted to share
the withheld code concurrently.
Although there's still a possibility for multiple m.no_olm withheld codes
to be sent out. This may happen during the propagation of the flag
remembering the fact that the `m.no_olm` code has been sent from the
OutboundGroupSession to Device. This scenario is likely unrealistic and
the consequences of sending things twice aren't problematic.
Currently, the tags used for releases of this binding have the format:
"matrix-sdk-crypto-js-v0.1.0-alpha.5". This is inconsistent with tags used for
other parts of the project, which omit the `v` prefix.
The url crate normalizes the string, but during OIDC verification steps,
the issuer verification must be made against the exact string that was
provided.
Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
Like for the state store, the bindings expose the version as an f64
while the web API expects an unsigned integer.
Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
This patch should ideally be split into multiple smaller ones, but life
is life.
This main purpose of this patch is to fix and to test
`SlidingSyncListRequestGenerator`. This quest has led me to rename
mutiple fields in `SlidingSyncList` and `SlidingSyncListBuilder`, like:
* `rooms_count` becomes `maximum_number_of_rooms`, it's not something
the _client_ counts, but it's a maximum number given by the server,
* `batch_size` becomes `full_sync_batch_size`, so that now, it
emphasizes that it's about full-sync only,
* `limit` becomes `full_sync_maximum_number_of_rooms_to_fetch`, so that
now, it also emphasizes that it's about ful-sync only _and_ what the
limit is about!
This quest has continued with the renaming of the `SlidingSyncMode`
variants. After a discussion with the ElementX team, we've agreed on the
following renamings:
* `Cold` becomes `NotLoaded`,
* `Preload` becomes `Preloaded`,
* `CatchingUp` becomes `PartiallyLoaded`,
* `Live` becomes `FullyLoaded`.
Finally, _le plat de résistance_.
In `SlidingSyncListRequestGenerator`, the `make_request_for_ranges`
has been renamed to `build_request` and no longer takes a `&mut self`
but a simpler `&self`! It didn't make sense to me that something
that make/build a request was modifying `Self`. Because the type of
`SlidingSyncListRequestGenerator::ranges` has changed, all ranges now
have a consistent type (within this module at least). Consequently, this
method no longer need to do a type conversion.
Still on the same type, the `update_state` method is much more
documented, and errors on range bounds (offset by 1) are now all fixed.
The creation of new ranges happens in a new dedicated pure function,
`create_range`. It returns an `Option` because it's possible to not be
able to compute a range (previously, invalid ranges were considered
valid). It's used in the `Iterator` implementation. This `Iterator`
implementation contains a liiiittle bit more code, but at least now
we understand what it does, and it's clear what `range_start` and
`desired_size` we calculate. By the way, the `prefetch_request` method
has been removed: it's not a prefetch, it's a regular request; it was
calculating the range. But now there is `create_range`, and since it's
pure, we can unit test it!
_Pour le dessert_, this patch adds multiple tests. It is now
possible because of the previous refactoring. First off, we test the
`create_range` in many configurations. It's pretty clear to understand,
and since it's core to `SlidingSyncListRequestGenerator`, I'm pretty
happy with how it ends. Second, we test paging-, growing- and selective-
mode with a new macro: `assert_request_and_response`, which allows to
“send” requests, and to “receive” responses. The design of `SlidingSync`
allows to mimic requests and responses, that's great. We don't really
care about the responses here, but we care about the requests' `ranges`,
and the `SlidingSyncList.state` after a response is received. It also
helps to see how ranges behaves when the state is `PartiallyLoaded`
or `FullyLoaded`.
The part of code that converts a list of users to the actual
/keys/query request uses the chunks() method. This method operates on
the slice. Our list/vec of users gets dereferenced into a slice before
we create our chunks. The chunks can't take ownership of the data, which
in turn requires us to clone the user IDs for them to be put into the
request.
Itertools has a chunks() method which operates on an iterator which we
can utilize to remove, not only the clone, but also a collect call.
At the same time, let's make the conversion step a simple functional
mapping and document what's going on.
The Option conveys our intention a bit better compared to the default
value. While nothing bad can happen, since the request IDs map will be
empty by default, it's a bit scary to have a valid sequence number since
an i64 defaults to 0.
This patch updates `SlidingSync.pos` and `.delta_token`. Each field has their
own lock. It's annoying because they must updated at the same time to not be
out-of-sync.
So a new field `SlidingSync.position` of type `SlidingSyncPositionMarkers` is
created. It holds `pos` and `delta_token. This new field is behind a single
`RwLock`.
The comments in the code should explain the general approach, but to give an overview:
We assign a unique sequence number to each device-list-invalidation notice, and keep track
of the sequence number at which each user was most recently invalidated. Then, when we
build a `/keys/query` request, we take note of the current sequence number. Finally, when
we get the response from that `/keys/query` request, we compare that sequence number
with the most-recent-invalidation of each user that is now supposedly up-to-date. All
of that means that we can see if the user has been invalidated *since* we built the
`/keys/query` request, in which case our request may have raced against the new device,
so we do not mark the user as up-to-date.
To make that work, we need to keep track of the sequence number for in-flight `/keys/query`
requests; we do that in the `IdentityManager`. Since there should only ever be at most one
batch of requests in flight, that is relatively easy; however, we also record the request ids
just to check that the application isn't doing something weird.
There is no need to persist the sequence numbers to permanent storage: provided the
`IdentityManager` and `Store` objects have a lifetime at least as long as an in-flight
`/keys/query` request, it is sufficient just to retain the `dirty` bit in permanent storage,
and then, when we reload the ephemeral `Store` cache, to treat everything with a `dirty`
bit as a "new" invalidation.
Fixes: #1386.
With https://github.com/matrix-org/matrix-rust-sdk/pull/1601, it's safer to
cancel a stream at any time, as any `Future` cancellation. This documentation
section of `SlidingSync` is no longer relevant as is, and can be rephrased.
With https://github.com/matrix-org/matrix-rust-sdk/pull/1601,
`TaskHandle::with_callback` is no longer necessary.
This patch updates `TaskHandle` as follows:
1. Before, the `handle` and `callback` fields were not mutually exclusive!
Indeed, the `callback` field was used as an abort mechanism, but _also_ as a
finalizer, i.e. a code that runs _after_ the cancellation of `handle`. Now
that the callback-based solution is no longer used, we can keep one usage for
this field and rename it `finalizer`.
2. The `handle` field is no longer optional, as it will always be set.
3. The `with_callback` method is renamed `new` as it's now the only constructor.
4. The `set_callback` method is renamed `set_finalizer`.
Tadaaa.
Move all `SlidingSync`'s fields into a new `SlidingSyncInner` type, and then
update `SlidingSync` to contain a single `inner: Arc<SlidingSyncInner>` field.
First off, it's much simpler to understand what's behind an `Arc` for
“clonability” of `SlidingSync`, and what's not. We don't need to worry about
adding a new field that is not behind an `Arc` for a specific reason or
anything. The pattern is clear now.
Second, this is required for next commits.
Before this patch, `SlidingSync::sync` was returning a callback-based
`TaskHandle`. It was waiting on the “stream loop” to finish (since it's a long-
poll, it means waiting 2*30s, cf. our code), before checking an atomic flag has
some value to decide whether it was time to leave the loop or not. So when the
user is cancelling this `TaskHandle`, a response (if any) was always handled.
But in the meantime, it was possible to start a new `sync`, and it seems like
it creates bugs.
After this patch, `SlidingSync::sync` now returns a handle-based `TaskHandle`.
It means that cancelling it will cancel the “stream loop” immediately. If
no response was in-flight from the server, that's perfect, no problem. If a
response was in-flight, the inner `pos` of the `SlidingSync` instance won't be
updated as the response won't be handled. So the server will re-send the same
response with the next sync request.
I guess it's better this way. Thoughts?
This patch adds a new feature.
Each call to `SlidingSync::stream` generates a unique `stream_id`. This
`stream_id` is passed to requests as a `txn_id` field. Returned responses must
hold the same `txn_id`, otherwise it means `stream` has received unexpected
responses from another `stream` or something else.
So far, when the `txn_id` field of the response and the `stream_id` don't
match, a log with the `ERROR` level is raised. Should we do more, like ignoring
the response entirely? Not sure yet.
The IndexedDB API expects an unsigned integer for the version.
The web-sys bindings expose it as a f64 so versions like 1.1 and 1.2
were used. The were converted back to uint in JS which means the version
was always 1.0 and no real upgrade was processed
Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
Simplify handling of sliding sync extensions
This patch simplifies the logic for handling sliding sync extensions.
These extensions are sticky, meaning that once enabled in one request,
they do not need to be repeatedly enabled for subsequent requests.
However, the previous logic tried to accommodate the case where the
extensions were initially disabled to reduce the size and processing
time of the initial response. This resulted in complex and error-prone
code.
With this patch, we have removed support for disabling to-device events
and streamlined the handling of sync extensions.
Co-authored-by: Jonas Platte <jplatte@matrix.org>
You can't update a GHA cache once you create it, so if we use the same cache
for each job in a matrix build, then we end up populating it for the first job
that completes, so any slower jobs don't get their dependencies cached.
On the other hand, if we create 20 500MB cache items on each build, we're going
to exhaust the cache storage as soon as we do a build. So, instead, let's just
do the caching for the main branch, and hope that other branches can still
benefit from it.
* Replace custom cancellation action with `concurrency`
* Improve step names
... so don't have three steps with the same name
* Bump version of checkout action
checkout@v2 uses an old version of nodejs, which is deprecated.
To declare the tests, the macro is still used but the content of the
tests is moved in the StateStoreIntegrationTests trait.
Allows to get the proper line reported when a panic occurs,
and have linting, formatting and IDE suggestions.
Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
Currently, the cache of the xtask binary isn't working terribly well:
* we seem to build it on each run anyway, presumably because we don't cache any of the intermediate build
artifacts. Running the binary directly rather than indirecting via "cargo" prevents this.
* There is no sharing of the cache between the "rust" and "bindings" CI, because we use different cache keys.
This PR addresses both problems, and hopefully speeds up CI a bit as a result.
The Sliding Sync server might not send some parts of the response, because they
were sent before and the server wants to save bandwidth. So let's update values
only when they exist.
Running the same example twice
borks encryption.
Instead point to an example that shows how to do it.
Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
This patch cleans up the `SlidingSync::stream` method. It renames variables,
improves log messages etc.
This patch also creates a new `MAXIMUM_SLIDING_SYNC_SESSION_EXPIRATION`
constant. This value was previously hardcoded and lost in the code, now it's
easier to spot it for further updates.
This patch finally renames the `failure_count` field to `reset_counter`,
because it doesn't count the number of failure, but the number of
`ErrorKind::UnknownPos` exactly, i.e. the number of times we reset the
`SlidingSync` state.
This patch cleans up the `SlidingSync::sync_once` method by renaming variables,
adding more comments, simplifying the code by reducing the number of variables
etc.
This patch also changes `futures_util::join!` to `futures_util::future::join`.
It does the same but the macro needs the `async-await-macros` feature to be
turned on, while the second works without any features.
Finally, this patch improves the log messages by making them more clear for a
new reader.
I admit this patch is quite tricky. Please try to follow me.
So, first off, in `SlidingSyncRoom::new`, we were clearing the timeline,
because somehow it exists twice in memory at this step.
Which led me to understand how `SlidingSync::handle_response` was working.
I've clarified how this part of the code works. We are dealing with 2 kind of
responses for a specific reason: `SyncResponse` and `v4::Response`, now it's
documented and I hope it's clearer.
Then, I notice that we were passing a clone of the entire sliding sync
response (`v4::Response`) to `Client::process_sliding_sync`. I thought it was
suboptimal, so I've updated the code to take a reference. It led me to update
`BaseClient::process_sliding_sync`. It was a little bit tricky, but I reckon we
have less clones now than before.
And now, back to `SlidingSync::handle_response`, I was able to compute the
`timeline` correctly by draining it from the `v4::Response`, or by moving it
from `SyncResponse`. So it's no longer necessary to have this clearing code
inside `SlidingSyncRoom::new`. Honestly it has nothing to do at this place
before.
To conclude: We have cleaner code, and less clones.
What thing I reckon could be optimized, is that the entire `timeline`
(`Vec<TimelineEvent>`) is cloned to be passed to `Client::handle_timeline`. So
this timeline exists in 2 places: in Sliding Sync, and somewhere else. I don't
believe it's a problem now, that's how it works, but we must be aware of that.
The `Client::login_custom` allows to login by using a custom login
method. In particular, it is possible to login to Synapse which supports
JWT authentication.
Signed-off-by: boxdot <d@zerovolt.org>
First off, it's not necessary for `SlidingSyncRoom::update` to take a reference
to `room_data: v4::SlidingSyncRoom`. When `update` is called, the iterator owns
its items.
Second, by taking ownership of `room_data`, we no longer need to clone all the
data we need to assign to `self.inner`.
First off, `SlidingSyncRoom.from` doesn't need to be visible to the entire
crate, only to `crate::sliding_sync.
Second, it's a constructor, so let's call it `new`.
1. Put `FrozenSlidingSyncRoom` at the bottom of the module.
2. Put merge 2 `impl SlidingSyncRoom` together.
3. Remove the `AliveRoomTimeline` type alias.
4. Improve the documentation.
So far, the `SlidingSync.pos` field was public to the crate. In order to avoid
breaking the internal state of this type, its visibility is now private.
However, we need to be able to change the value when testing the
`SlidingSync` type itself. To achieve that, this patch removes the old
`force_sliding_sync_pos` function, and implements 2 new functions: `set_pos`
and `pos` directly on `SlidingSync` only when `#[cfg(any(test, feature ="testing"))]`.
Previously, it was possible for us to use invalid indices when:
- We retried decrypting multiple events at once
- One of them (not the last) was an edit or reaction
This lead to a situation where we would remove the UTD item once
decryption for it was successfully retried, but not account for the
resulting index shift for all later timeline items.
Before this patch, `SlidingSyncView` has the following fields that were public:
`state`, `rooms_list` and `rooms_count`. Since they are `Mutable`, they can be
changed from the outside, and then will break the internal state of the view.
This problem is mentioned in https://github.com/matrix-org/matrix-rust-sdk/
issues/1474.
This patch solves this by making them prviate. Phew. That was simple!
But wait, we have a problem now. `matrix-sdk-ffi` was relying on them. So
this patch adds “helpers” methods on `SlidingSyncView`, like `state_stream`,
`rooms_list`, `rooms_list_stream`, `rooms_count` and `rooms_count_stream`.
Let's add new ones when it's necessary.
In https://github.com/matrix-org/matrix-rust-sdk/pull/1478, I've introduced
a regression. The FrozenSlidingSyncRoom.timeline` field has been renamed to
`timeline_queue`.
I didn't notice that this type can be serialized and deserialized. That's
actually a breaking change because this type is stored somewhere once
serialized. So with #1478, it was impossible to deserialize them!
This patch fixes that. It also adds a test to ensure the serialization form of
`FrozenSlidingSyncRoom` doesn't change.
This patch allows an `OlmMachine` to use a different store than Sled, like SQLite.
A new enum is introduced: `StoreType` which 2 variants: `Sled` (the default), and `Sqlite`.
The `OlmMachine.initialize` constructor now takes a new optional `store_type:
Option<StoreType>` argument. If no value is passed, the default `StoreType`
variant will be used (as mentioned: `StoreType.Sled`).
The code has been rewritten a little bit to make the type system happy without
introducing too much type indirections.
This patch finally adds a parameterized tests that exhaustively test: no store
type, `Sled` and `Sqlite`.
Building the crypto-js bindings in release mode is very slow and not really
necessary for local development.
`--release` is the default, so there is no need to specify it
explicitly. Instead, allow `wasm-pack` args to be specified by an env var, and
add a `build:debug` npm script which will build in debug mode.
We already have support for proper `ToDeviceRequest` objects in
`outgoing_requests`, so let's use it in `share_room_key` as well. It's much
easier for the application to work with the proper object than an untyped json
blob.
In the code `if let Some(global_data) = account_data.as_ref().map(|a|
&a.global)`, the `map` is not purely necessary. This patch simplifies the code
to remove the `map` and read the `global` field later on.
By using `Default::default` for the parameter value, we don't know which type
we are passing to the function. Hence the useful comment `room_info.ephemeral`.
This patch changes this to `Ephemeral::default`, so that we now know what we
pass without the need of a comment.
Javascript's `console.trace` is not a good equivalent for Rust's
`Level::TRACE`.
In particular:
* `console.trace` causes a stacktrace to be written with each message
* Under nodejs, `console.trace` writes the message to `stderr`, while
`console.debug` writes to stdout
`console.trace` is therefore somewhat analogous to `console.error`.
Since we already include the log level in the message, we might as well just
send trace messages to `console.debug` rather than `console.trace`.
This is useful for platforms which might want to avoid the cost of
password based key-derivation and have the ability to securely store an
encryption key.
Co-authored-by: Denis Kasak <dkasak@termina.org.uk>
Contrary to other tables that use their own name to encode their keys,
the `ROOM_INFO` table uses the `ROOM` table name to encode its keys.
Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
`wasm_timer` doesn't work outside the browser, which prevents it being used in
tests.
We can replace it with `gloo-timers` wich uses the standard Javascript
`setTimeout`.
This makes it easier to implement it. However, using a different error
type than CryptoStoreError is a non-trivial change, so left for the
coming commits for all stores except the memory store.
The key encoding in key-value stores is backwards-compatible so
old receipts are counted as unthreaded receipts.
Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
A fix for (de)serialization of redacted state events in Ruma made
old events undeserializable.
Bump the DB version.
Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
A fix for (de)serialization of redacted state events in Ruma made
old events undeserializable.
Bump the DB version.
Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
This implementation is wrong in the sense of its semantics is not about
dereferencing a thin pointer to something, but just to give access to one
specific field of the entire structure. That's not how `Deref` is supposed to
be used.
Moreover, it creates conflict between the `SlidingSyncRoom.timeline` field,
and `SlidingSyncRoom.inner.timeline` field, which both exist, but not for the
same purposes. It creates confusion in the code.
Finally, it's better to expose proper getters to the outside world, so that we
control _and_ test _and_ know exactly what API we provide.
`Timeline::with_fully_read_tracking` was public although all public
Timeline constructors enable it by default.
It is also meant to be called when constructing the timeline so
using a builder pattern makes sense.
Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
`SlidingSyncView` exposes a `room` field. Problem: It's not updated
correctly and leads to error. The canonical way to get a room is by using
`SlidingSync::get_room`. This patch thus removes `SlidingSyncView.rooms`
entirely.
Before this patch, `SlidingSync::get_room` was taking ownership of an
`OwnedRoomId`, to only use it as a reference. Thus, calling this method was
creating useless clones.
This patch updates `SlidingSync::get_room` to receive a reference to
`OwnedRoomId`.
Note about "Write-Ahead Log" (WAL) mode: The SQLite WAL mode has a
bunch of advantages that are quite nice to have:
1. WAL is significantly faster in most scenarios.
2. WAL provides more concurrency as readers do not block writers and a
writer does not block readers. Reading and writing can proceed
concurrently.
3. Disk I/O operations tends to be more sequential using WAL.
4. WAL uses many fewer fsync() operations and is thus less vulnerable
to problems on systems where the fsync() system call is broken.
The downsides of WAL mode don't really affect us. So let's turn it on.
More info: https://www.sqlite.org/wal.html
Co-authored-by: Jonas Platte <jplatte@matrix.org>
Co-authored-by: Damir Jelić <poljar@termina.org.uk>
When we store an Olm session in the database, we encode the sender key and
session ID using the store cipher. That means that we also need to encode them
when we look them up, otherwise we won't find them.
This patch is trying to resolve the following issue:
When a local event is sent to the server, in case of an error, the
`Timeline::send` method just returned the error but it wasn't reflected inside
the `LocalEventTimelineItem` type itself. It's easy to loss this information.
So now, there is a new enum: `LocalEventTimelineItemSendState` with 3 states:
1. `NotSentYet`, when the local event has not been sent yet, it's theorically
the initial state,
2. `SendingFailed`, when the local event has been sent but it's failed!
3. `Sent`, when the local event has been sent successfully.
Therefore, the `LocalEventTimelineItem` struct has a new field: `send_state`.
The send state is managed by its `with_event_id` method which now receives an
`Option<OwnedEventId>` (prev. `OwnedEventId` directly):
* If it's `Some(_)`, then it means we got a successful response from the server
after the sending, so the send state is set to `Sent`,
* If it's `None`, then it means we got an error response from the server, so
the send state is set to `SentFailed`.
(A small change: The method `TimelineInner::add_event_id` has been renamed
`TimelineInner::update_event_id_of_local_event`.)
The `Timeline::send` still returns a `Result`, but the server's response is
passed to a new method: `TimelinerInner::handle_local_event_send_response`
(it's logically named after the `handle_local_event` method), which is
responsible to call `TimelineInner:update_event_id_of_local_event` (which was
previously called directly by `Timeline::send`).
And `TimelineInner::update_event_id_of_local_event` was already calling
`LocalEventTimelineItem::with_event_id`. So everything works like a charm here.
The local send state is closely managed by `LocalEventTimelineItem`, I hope it
will avoid state breakage as much as possible.
The sliding sync logic has another room type called `SlidingSyncRoom`.
This room type stores a limited amount of events in something called
`AliveRoomTimeline` in a in-memory cache. This room also gets persisted
to the store via another room type called `FrozenSyncRoom`.
These types are used when clients restore their view of the room, i.e.
when the user enters the room to look at messages.
When the client enters a room, a `Timeline` object will be created, but
it will be initially populated with events coming from
`AliveRoomTimeline`.
In a hot potato contest of who should be responsible to decrypt injected
events, everybody claims to be allergic to potatoes.
This patch modifies the `Timeline` constructor that populates the
timeline with events from the `AliveTimeline` to retry decryption on the
events that the `AliveTimeline` pushes into the `Timeline`.
Note that, because the `AliveRoomTimeline` never replaces encrypted
events with decrypted ones, every time the client enters/exits the room
events well get re-decrypted.
This is an omission from the spec where the subkeys are supposed to be
signed by the master key, but signing the master key wasn't mentioned.
This is still fine, since we always treated the master key and their
subkeys as a whole and never accepted one without the others.
To create an event next to the server, the event must be accompanied by
a transaction ID. When the server receives the event creation request, it
responds with a new event ID (which we will name `event_id_1`). Later on, when
a sync is done to the server, the server may respond with the transaction ID
and a new event ID (which we will name `event_id_2`).
The transaction ID is used to update the state of the local event. The event
ID received from the sync may ideally be the same as the one received by the
creation request's response.
Knowing that…
`EventTimelineItem` had a field: `event_id: Option<OwnedEventId>`. It was
`Some(event_id)` where `event_id` is the event ID responded by the server when
an event was created, i.e. it's `event_id_1`; otherwise it was `None`. This is
never `event_id_2`. Why?
Because `EventTimelineItem` has an other field: `key: TimelineKey`, which
represents the following states:
* `TimelineKey::TransactionId(OwnedTransactionId)`,
* `TimelineKey::EventId(OwnedEventId)`, where the event ID is `event_id_2`!
So it becomes obvious that `event_id_1` should be part of `TimelineKey`, not
`EventTimelineItem`. `TimelineKey` is where this transaction ID and event ID
logic is managed.
This patch updates `TimelineyKey::TransactionId` to be defined as:
* `TimelineKey::TransactionId { txn_id: OwnedTransactionId, event_id: Option<OwnedEventId> }`.
Why an `Option`? Because before receiving `event_id_1`, we may still want to
create a `TimelineKey`, the API must reflect that state.
So basically, `EventTimelineItem.event_id` is moved inside
`TimelineyKey::TransactionId`.
… rather than prepended, or added at a specific point as replacement for
an existing timeline item.
Previously, we would log lots of useless tracing events when retrying to
decrypt UTD items.
This patch fixes https://github.com/matrix-org/matrix-rust-sdk/issues/1379/.
This is similar to https://github.com/matrix-org/matrix-rust-sdk/pull/1197/.
We need to close the `OlmMachine`. Problem, `napi-rs` doesn't allow methods to
take ownership of `self`, so the following code:
```rs
fn close(self) {}
````
isn't allowed. There an [`ObjectFinalize`](https://docs.rs/napi/latest/napi/bindgen_prelude/trait.ObjectFinalize.html)
trait, but it's only called when the JS value is being collected by the GC.
It's not possible to force call `finalize` here.
This patch then introduces a new type:
```rs
enum OlmMachineInner {
Opened(matrix_sdk_crypto::OlmMachine),
Closed
}
```
that derefs to `matrix_sdk_crypto::OlmMachine` when its variant is `Opened`,
otherwise it panics. Calling the new `OlmMachine::close` method will change the
inner state from `Opened` to `Closed`.
Ideally, I wanted to throw an error instead of panicking, but `napi_env`
(used to build an `Env`, used to throw an error) is neither `Sync` nor `Send`.
Honestly, my knowledge of NodeJS and NAPI is too weak to implement `Sync`
and `Send` for `*mut napi_env__` safely. Especially because it can be executed
from various threads within `async fn` functions, that are driven by Tokio in
`napi-rs`. So, yeah, for the moment, it panics!
This moves the caches we have for tracked users out of the Sled, and
IndexedDB, CryptoStore into the Store struct. The Store struct is a
wrapper for the CryptoStore trait, so this is the natural place where
these caches should live.
Co-authored-by: Jonas Platte <jplatte@matrix.org>
Before we'd push one empty entry at a time, which lead to a lot of vecdiff updates pushed. With this
we now only push one event after the entire operation set has been applied on the fresh
or cold view.
A mismatch between the recorded Ed25519 and Curve25519 keys of the room
key and the identity keys of a Device used to be just a warning.
Considering that many clients will aggressively try to hide E2EE related
warnings let's upgrade this clear error into a full blown decryption
failure.
Co-authored-by: Denis Kasak <dkasak@termina.org.uk>
We might have a lot of tracked users, clients will usually have 1-10
thousand tracked users. This might slow down client startup if there are
so many tracked users.
We don't need the tracked users until we sync or try to send a message,
so load them lazily when they are first needed.
The conventional commit specification recommends the "docs" type for
documentation related changes. This also matches the initial usage of
this type in our commit history.
While the default git-cliff config does specify a regex "^doc", where I
suspect our types have been taken from, this regex actually allows "doc"
as well as "docs" for the type.
Nevertheless, let's specify that we're using the one recommended in the
spec.
I had no idea what a tracked user was, so I've attempted to clarify this
somewhat for future readers.
Co-authored-by: Damir Jelić <poljar@termina.org.uk>
Instead of putting all request fields inside a single `body` JSON-
encoded string field, there is now more types. There is less need to call
`JSON.parse`, and the type system will be able to catch more things.
For example, `ToDeviceRequest` now has 2 more fields: `event_type` and
`txn_id`.
This solves the issue of correlating different log lines that are talking
about the same request/response cycle. Plaintext logs would show this as
a flat structure, while something like Jaeger would show you a nice
tree. This allows you to reconstruct the tree in plaintext logs.
Various smaller fixes on sliding-sync for elem-x:
- Infrastructure and smoke test only for integration testing sliding-sync
- Expose Pop and Clear on VecDiff
- Expose adding views during sliding sync live runtime incl integration tests
- bug fix on replacing only in-window-items in the insert-code (was leading to a wrong list setting), including integration test
Required when `PaginationOptions` must implement `Send`,
e.g. with `tokio::runtime::Runtime::spawn`.
Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
When updating a device after we receive new device keys from a
/keys/query call we implicitly check that the user and device id match
since we need to fetch the device using the mentioned ids.
This moves the check into the update method as well to make it more
explicit. We also prevent upgrades of the Ed25519 key since we don't
support any different key type anyways.
The sliding sync proxy seems to have started to omit the e2ee extension
completely if no changes happened to the one-time key counts.
Our sync response processing assumed that, if there are some to-device
events that need to be passed to the OlmMachine, then we'll have some
data in the e2ee extension of the response as well. In other words, it
wouldn't pass in the to-device events to the OlmMachine if the e2ee
field of the sync response is `None`.
Since the assumption isn't true, we're going to pass in default values
for the one-time key counts when the e2ee extension field of the
response is `None`.
This is mainly done so we create a span with the name of the function,
since we can filter logs by the name of the span we're now able to
enable logs for this method only.
Previously, when we found a since-token in the frozen state, we'd always activate the to-device extensions
regardless of whether the user had activated it in their builder or didn't. With this change we are only
setting the since parameter from cache if the user actually activated the to-device extension.
- Don't actually fire off a request when the top of the timeline has
already been reached
- Allow making multiple requests without removing the loading indicator
in between
Index ranges are inclusive, but our loop would stop one short. This particuarly
tricky when the selective view is moved, as we didn't properly invalidate all items.
It's not a hard-and-fast-rule that Javascript exceptions should be `Error`
instances, but it's certainly a convention, and one that is going to reduce
surprises if we follow.
Add a second full-sync-up mode to sliding sync. Previously - and still the default for backwards compatibility, but now named `PagingFullSync` - was to page through the list by the page-size, de-validating the previous page of items. The newly added `GrowingFullSync` instead grows the window by the given number `batch_size` per request, starting and keeping it from `0`. In the latter we might be pushing more data over the connection and are slightly slower, but the top always stays active and thus reactive to changes.
Furthermore the developer can now configure an optional maximum number to grow/paginate the full-sync up to (so stopping before actually having reached `count` whatever is smaller). This is already exposed via FFI, too.
- [x] add new full-sync-mode
- [x] add limited sync-up mode (similar to full sync but only to a limit `n`)
- [x] implement sync-up in jack-in
Sliding Sync updates
- expose timeline listener via FFI on slidingsyncroom, too - returning a stoppable spawn
- connect room timeline and sliding-sync-room timeline together
- increase HTTP timeout on jack-in
- add jack-in support for actual timeline items.
We automatically request room keys to be forwarded from our other trusted devices if a decryption failure happens because the room key is missing.
This patch introduces automatic room key requests for decryption failures if the room key is available but has been ratcheted forward. In other words, we will now request a better version of the given room key automatically as well.
Co-authored-by: Damir Jelić <poljar@termina.org.uk>
`open_with_db` also has the passphrase parameter without mentioning it
in its name. The old name also sounded like the passphrase was required
when it's actually optional.
Clarify that the JSON-encoded `toDeviceEvents` passed to
`OlmMachine.receiveSyncChanges` must be the list of events themselves,
instead of a wrapper object that contains the event list.
Signed-off-by: Andrew Ferrazzutti <andrewf@element.io>
Tokens are not necessary for the restoration of the crypto/store, only
the meta. Required for OpenID Connect support where we need the tokens to get the
meta.
When the verification support was initially bound, Uniffi did not
support objects (the OlmMachine) returning other objects (our
verification objects). Instead we converted all the verification objects
into pure data structs which the other side had to poll for changes.
Now that Uniffi supports returning objects we can refactor this and make
the API on the other side the same as on the Rust side.
With these changes, the user of sliding sync can configure the SlidingSync-Builder to store and recover the state from storage. For that they should call cold_cache("my-sliding-sync-key") with their preferred storage location during setup time on SlidingSyncBuilder. If a cached version is found under that key, the internal state of the rooms-list as well as each view found (identified by their name) will be set from storage immediately - allowing the user to show the last cached state. The Builder then also uses the same key to store its latest state after every update received from remote.
👉 Note on room list:
As we store a disconnected state, we are saving all RoomList entries as either Empty or Invalidated. This allows for an easier updating when the server comes back with results, as we don't track the ranges in cache - in the view of the server, we haven't seen anything yet.
👉 Note on timeline events:
This does store all existing timeline events - initial and seen during the session (as we keep track of them right now) - and will recover that state. However, as we can't be sure wether we have gaps in the timeline, the timeline items are reset upon receiving updates for them from the server.
👉 Note on (full-sync-)view:
While we are caching the server returned results (view list, room count and room info) as is, we are not caching the internal state (whether we are catching up) nor the ranges (as they are likely to be out of date) - those will be acting the same way you configured them, just with preloaded results. So for a full-sync-view, it will still do requests in batches and replace the corresponding set of rooms. Which could mean you might see the same room appear twice, though the cached one would be marked as invalidated. This might be problematic if the list the UI shows is longer than the batches fetched, but would be resolved quickly when catching up.
👉 On recovery:
When the sliding sync receives a M_UNKNOWN_POS, indicating the server has expired our session, sliding sync now transparently retries with up to three times to restart the sliding sync with full set of extensions and the latest views at their existing windows, the current room state is held. For full sync this starts a new sync-up with the existing room list staying intact. This also works from the offline at start.
- matrix_sdk_base::sync::SyncResponse is the internal representation that
we can update to account for sliding sync
- matrix_sdk::sync::SyncResponse is `/v3/`-specific and should not change
* Move swift build scripts into xtask (#1201)
* fix(ffi): use target_path from `cargo metadata` rather than guessing
* ci(ffi): install necessary target arch for build-framework test
* feat(xtask): copy to target without rsync.
As it appears, the first sync for a room might not include the
encryption state, so we cannot set the encryption state to be synced on
incoming syncs. That way, we always fetch the encryption state manually.
* split logger into platform specific implementations
* logging support for android
* use platform independent wrapper for uniffi:exports
* activate colors for ansi-terminals
It turns out that Google Chrome refuses to initialise the wasm via the
synchronous `WebAssembly.Module` constructor, complaining that it is too
big. To be fair, it has a point.
Anyway, that means we need to provide a way to load the wasm
asynchronously. So, we introduce an `initAsync()` function which applications
can call before they do anything else, to load the wasm in the background.
If the app *doesn't* call `initAsync()`, then we load the wasm synchronously
the first time a function that accesses the wasm is called.
This new `OlmMachine.close` forces to drop/close the `OlmMachine` without
waiting on the JavaScript garbage collector to collect it.
`wasm-bindgen` generates the following JS glue code:
```js
close() {
const ptr = this.__destroy_into_raw();
wasm.olmachine_close(ptr);
}
```
And, `__destroy_into_raw` looks like this:
```js
__destroy_into_raw() {
const ptr = this.ptr;
this.ptr = 0;
OlmMachineFinalization.unregister(this);
return ptr;
}
```
It unregisters itself from the `FinalizationRegistry` correctly. We are
protected from a double-free.
Surprisingly, `indexed_db_futures::IdbDatabase` is not closed when dropped.
Hopefully, there is a [`IdbDatabase::close(&self)`][close] method, which calls
`web_sys::IdbDatabase.close`, aka [`IDBDatabase.close`][websys-close], so let's
use it!
`IDBDatabase.close` returns immediately and closes the connection in a separate
thread. The connection isn't actually closed until all transactions created
using this connection are complete. No new transactions can be created for this
connection once this method is called. Methods that create transactions throw
an exception if a closing operation is pending.
[close]: https://github.com/Alorel/rust-indexed-db/blob/8c106eb418aecdba2f3fde80d91a4673a875fdf6/src/idb_database.rs#L73-L77
[websys-close]: https://developer.mozilla.org/en-US/docs/Web/API/IDBDatabase/close
I have tried for hours and have concluded it is probably not possible
even with GATs to have RawEvent borrow its inner value.
It is also not a commonly-used feature, so removing the unnecessary
clones is not that important.
This patch adds a couple more states to the SAS verification state
machine. Namely we add the following states:
* KeySent
* KeysExchanged
This prevents users from confirming that the short auth string matches
before they have sent out a m.key.verification.key event, which is
necessary for the other side to present the short auth string.
The KeysExchanged state functionally replaces the KeyReceived state.
Meaning that the short auth string can only presented once we
transitioned into the KeysExchanged state.
We can transition into the KeysExchanged state from the KeyReceived
state or from the KeySent state, depending on if we first receive a
m.key.verification.key event or if we first send out our own
m.key.verification.key event.
This means that, if the transition into the KeysExchanged state happens
through the KeySent state, users won't be able to tell if the transition
happened. In other words, listening to `m.key.verification` events
doesn't work anymore, users will need to use the new `Sas::changes()`
API to listen to a stream of state changes.
`wasm_bindgen::JsValue::(from|to)_serde` now emit warnings because of a
circular dependency. To solve this problem, this patch now uses `gloo-utils`,
see https://rustwasm.github.io/wasm-bindgen/reference/arbitrary-data-with-serde.html#an-alternative-approach---using-json.
Ideally, we would like to use `serde_wasm_bindgen` but the behaviour isn't
the same. `gloo-utils` serializes and deserialized through JSON, whilst
`serde_wasm_bindgen` manipulates the `JsValue` directly which can be better or
worst dependending of the case. This patch conserves the JSON approach as it
was the previous and tested one.
By asking `wasm-bindgen` to generate JS glue code for `WeakRef` support,
it removes the need to call `free` manually to free objects, thus we reduce
potential memory leaks inside Rust. See https:// rustwasm.github.io/docs/wasm-
bindgen/reference/weak-references.html to learn more.
It uses [`FinalizationRegistry`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/FinalizationRegistry)
under the hood, I reckon it's quite common and fits into Matrix's clients needs
in terms of browser support.
While technically a type constructor can return a `Promise`, it's not
considered as idiomatic JavaScript to do that. We are changing `new OlmMachine`
to `OlmMachine.initialize`. The rest of the code is strictly the same.
Add general extension framework for sliding sync, implementing the e2ee, to-device and account-data extensions as per existing proxy implementation. Add a new (ffi exposed) function to use activate the extensions.
Also extends jack-in to have permanent login and storage support now, rather than posting an access token and expose messages inside the sliding-sync layer to actually use the decrypted messages if given. Contains a lot of fixes around these aspects, too, like uploading any remaining messages from the olm-machine on every sliding-sync-request or processing even if no room data is present (which can happen now as processing only extensions might takes place).
This patch adds a way for users to listen to changes in the state of a
SAS verification.
This makes it much more pleasant to go through the verification flow and
incidentally easier to document it.
Now that we're not scoping the room keys by the Curve25519 sender key
we're opening the door of multiple devices trying to insert the same
room key into our store.
This patch changes our logic so we only store room keys from an
m.room_key event if we don't have one already or if the new key is
a better version of the one we already have.
This mostly assumes that the first room key with a given session id
is coming from the creator of the room key.
It was very confusing that matrix_sdk::store::make_store_config's first
argument was either a path or a database name depending on the build
configuration. ClientBuilder::{sled_store, indexeddb_store} are also
easier to use.
@@ -51,7 +51,7 @@ You can stay informed about updates on the access token by listening to `client.
- [`MessageOptions`][message options] has been updated to Matrix 1.3 by making the `from` parameter optional (and function signatures have been updated, too). You can now request the server sends you messages from the first one you are allowed to have received.
-`client.user_id()` is not a `future` anymore. Remove any `.await` you had behind it.
-`verified()`, `blacklisted()` and `deleted()` on `matrix_sdk::encryption::identities::Device` have been renamed with a `is_` prefix.
-`verified()` on `matrix_sdk::encryption::identities::UserIdentity`, too has been prefixed with `is_` and thus is onw called `is_verified()`.
-`verified()` on `matrix_sdk::encryption::identities::UserIdentity`, too has been prefixed with `is_` and thus is now called `is_verified()`.
- The top-level crypto and state-store types of Indexeddb and Sled have been renamed to unique types>
-`state_store` and `crypto_store` do not need to be boxed anymore when passed to the [`StoreConfig`][store config]
- Indexeddb's `SerializationError` is now `IndexedDBStoreError`
**matrix-rust-sdk** leverages [UniFFI](https://mozilla.github.io/uniffi-rs/) to generate bindings for host languages (eg. Swift and Kotlin).
Rust code related with bindings live in the [matrix-rust-sdk/bindings](https://github.com/matrix-org/matrix-rust-sdk/tree/main/bindings) folder.
Developers can expose Rust code to UniFFI using two different approaches:
- Using an `.udl` file. When a crate has one, you find it under the `src` folder (an example is [here](https://github.com/matrix-org/matrix-rust-sdk/blob/main/bindings/matrix-sdk-ffi/src/api.udl)).
- Add UniFFI directivies as Rust attributes. In this case Rust source files (`.rs`) contain attributes related to UniFFI (e.g. `#[uniffi::export]`). Attributes are preferred, where applicable.
## Expose Rust definitions to UniFFI
### Check if the API is already on UniFFI
First of all check if the Rust definition you are looking for exists on UniFFI already. Most of exposed matrix definitions are collected in the crate [matrix-sdk-ffi](https://github.com/matrix-org/matrix-rust-sdk/tree/main/bindings/matrix-sdk-ffi).
This crate contains mainly small Rust wrappers around the actual Rust SDK (e.g. the crate [matrix-sdk](https://github.com/matrix-org/matrix-rust-sdk/tree/main/crates/matrix-sdk))
If the Rust definition is on UniFFI already, you either:
- find it in a `.udl` file like [matrix-sdk-ffi/src/api.udl](https://github.com/matrix-org/matrix-rust-sdk/blob/main/bindings/matrix-sdk-ffi/src/api.udl)
- see it marked with a proper UniFFI Rust attribute like this `#[uniffi::export]`
### Adding a missing matrix API
1. Unless you want to contribute on the crypto side, you probably need to add some code in the [matrix-sdk-ffi](https://github.com/matrix-org/matrix-rust-sdk/tree/main/bindings/matrix-sdk-ffi) crate. After you find the crate you need to understand which file is best to contain the new Rust definition. When exposing new matrix API often (but not always) you need to touch the file [client.rs](https://github.com/matrix-org/matrix-rust-sdk/blob/main/bindings/matrix-sdk-ffi/src/client.rs)
2. Identify the API to expose in the target Rust crate (typically in [matrix-sdk](https://github.com/matrix-org/matrix-rust-sdk/tree/main/crates/matrix-sdk). If you can’t find it, you probably need to touch the actual Rust SDK as well. In this case you typically just need to write some code around [ruma](https://github.com/ruma/ruma) (a Rust SDK’s dependency) which already implements most of the matrix protocol
3. After you got (by finding or writing) the required Rust code, you need to expose to UniFFI. To do that just write a small Rust wrapper in the related UniFFI crate (most of the time is **matrix-sdk-ffi**) you found on step 1.
4. When your new (wrapping) Rust definition is ready, remember to expose it to UniFFI.
It’s best to do it using UniFFI Rust attributes (e.g. `#[uniffi::export]`). Otherwise add the new definition in the crate’s `.udl` file. For the **matrix-sdk-ffi** crate the definition file is [api.udl](https://github.com/matrix-org/matrix-rust-sdk/blob/main/bindings/matrix-sdk-ffi/src/api.udl). **Remember**: the language inside a `.udl` file isn’t Rust. To learn more about how map Rust into UDL read [here](https://mozilla.github.io/uniffi-rs/udl_file_spec.html)
## FAQ
**Q**: I wrote my Rust code and exposed it to UniFFI. How can I check if the compiler is happy?\
**A**: Run `cargo build` in the crate you touched (e.g. matrix-sdk-ffi). The compiler will complain if the Rust code and/or the `.udl` is wrong.
**Q**: The compiler is happy with my code but the CI is failing on GitHub. How can I fix it?\
**A**: The CI may fail for different reasons, you need to have a look on the failing GitHub workflow. One common reason though is that the linter ([Clippy](https://github.com/rust-lang/rust-clippy)) isn’t happy with your code. If this is the case, you can run `cargo clippy` in the crate you touched to see what’s wrong and fix it accordingly.
This project and build script demonstrate how to create an XCFramework that can be imported into an Xcode project and run on Apple platforms. It can compile and bundle an [entire SDK](#Building-the-SDK), or only a smaller [Crypto module](#Building-only-the-Crypto-SDK) that provides end-to-end encryption for clients that already depend on an SDK (e.g. [Matrix iOS SDK](https://github.com/matrix-org/matrix-ios-sdk))
## Prerequisites for building universal frameworks
## Building
* the Rust toolchain
* UniFFI - `cargo install uniffi_bindgen`
* Apple targets (e.g. `rustup target add aarch64-apple-ios`)
*`xcodebuild` command line tool from [Apple](https://developer.apple.com/library/archive/technotes/tn2339/_index.html)
*`lipo` for creating the fat static libs
### Prerequisites for building universal frameworks
-`xcodebuild` command line tool from [Apple](https://developer.apple.com/library/archive/technotes/tn2339/_index.html)
-`lipo` for creating the fat static libs
### Building the SDK
```
sh build_xcframework.sh
cargo xtask swift build-framework
```
The `build_xcframework.sh` script will go through all the steps required to generate a fully usable `.xcframework`:
The `build-framework` task will go through all the steps required to generate a fully usable `.xcframework`:
1. compile `matrix-sdk-ffi` libraries for iOS, the iOS simulator, MacOS, and Mac Catalyst under `/target`. Some targets are not part of the standard library and they will be built using the nightly toolchain.
1. compile `matrix-sdk-ffi` libraries for iOS, the iOS simulator and macOS under `/target`. Some targets are not part of the standard library and they will be built using the nightly toolchain.
2.`lipo` together the libraries for the same platform under `/generated`
3. run `uniffi` and generate the C header, module map and swift files
4.`xcodebuild` an `xcframework` from the fat static libs and the original iOS one, and add the header and module map to it under `generated/MatrixSDKFFI.xcframework`
5. cleanup and delete the generated files except the .xcframework and the swift sources (that aren't part of the framework)
## Building only the Crypto SDK
For development purposes, it will additionally generate a `Package.swift` file in the root of the repo that can be used to add the framework to your project and enable debugging through the use of [rust-xcode-plugin](https://github.com/BrainiumLLC/rust-xcode-plugin) (make sure to run the task with the argument `--profile=reldbg`).
When building the SDK for release you should pass the `--release` argument to the task, which will strip away any symbols and optimise the created binary.
### Building only the Crypto SDK
```
sh build_crypto_xcframework.sh
build_crypto_xcframework.sh
```
The `build_crypto_xcframework.sh` script will go through all the steps required to generate a fully usable `.xcframework`:
@@ -38,16 +43,31 @@ The `build_crypto_xcframework.sh` script will go through all the steps required
4.`xcodebuild` an `xcframework` from the fat static libs and the original iOS one, and add the header and module map to it under `generated/MatrixSDKCryptoFFI.xcframework`
5. cleanup and delete the generated files except the .xcframework and the swift sources (that aren't part of the framework)
## Running the Xcode project
### Building & testing the Swift package
The Xcode project is meant to provide a simple example on how to integrate everything together but also a place to run unit and integration tests from.
The `Package.swift` file in this directory provides a simple example on how to integrate everything together but also a place to run unit and integration tests from.
It's pre-configured to link to the generated .xcframework and .swift files so successfully running the script first is necessary for it to compile.
It makes the compiled code available to swift by importing the C header through its bridging header.
Once all the generated components are available running it should be as easy as choosing a platform and clicking run.
It's pre-configured to link to the generated static lib and .swift files so successfully running `cargo xtask swift build-library` first is necessary for it to compile. Afterwards you can execute the tests with `swift test`. Note that for the moment this only works on macOS but we're planning to add Linux support in the future.
## Distribution
The generated framework and Swift code can be distributed and integrated directly but in order to make things simpler we bundle them together as a Swift package available [TBD](here) in the case of SDK, and as CocoaPods podspec in the case of Crypto SDK.
The generated framework and Swift code can be distributed and integrated directly but in order to make things simpler we bundle them together as a [Swift package](https://github.com/matrix-org/matrix-rust-components-swift/) in the case of SDK, and as a CocoaPods podspec in the case of Crypto SDK.
### Publishing MatrixSDKCrypto
1. Navigate into `bindings/apple` and run [`build_crypto_xcframework.sh`](#building-only-the-crypto-sdk) script which generates a .zip file with the framework in `./generated` folder
Note that whilst you can run this command from any git branch, if you want to make a public release, ensure you are building from latest `main`
2. Tag the commit you just used to build the release and push the tag to GitHub
Use `matrix-sdk-crypto-ffi-<major>.<minor>.<patch>` tag name
3. Create a new [GitHub release](https://github.com/matrix-org/matrix-rust-sdk/releases) using this tag (see [example](https://github.com/matrix-org/matrix-rust-sdk/releases/tag/matrix-sdk-crypto-ffi-0.3.4))
4. Upload the previously generated .zip file to this release
5. Increment the version in [`MatrixSDKCrypto.podspec`](./MatrixSDKCrypto.podspec)
Note that this is not automated and is a local-only change. To determine the most recent published version, run `pod repo update && pod search MatrixSDKCrypto`
or check git tags via `git tag | grep matrix-sdk-crypto-ffi`
6. Push new Podspec version to Cocoapods via `pod trunk push MatrixSDKCrypto.podspec --allow-warnings`
// base64 decoder, based on the code at https://developer.mozilla.org/en-US/docs/Glossary/Base64#solution_2_%E2%80%93_rewriting_atob_and_btoa_using_typedarrays_and_utf-8
/// State machine implementation of the Olm/Megolm encryption protocol
@@ -28,25 +33,38 @@ pub struct OlmMachine {
#[wasm_bindgen]
implOlmMachine{
/// Create a new memory based `OlmMachine`.
/// Constructor will always fail. To create a new `OlmMachine`, please use
/// the `initialize` method.
///
/// The created machine will keep the encryption keys only in
/// memory and once the objects is dropped, the keys will be lost.
/// Why this pattern? `initialize` returns a `Promise`. Returning a
// `Promise` from a constructor is not idiomatic in JavaScript.
#[wasm_bindgen(constructor)]
pubfnnew()-> Result<OlmMachine,JsError>{
Err(JsError::new("To build an `OlmMachine`, please use the `initialize` method"))
}
/// Create a new `OlmMachine`.
///
/// `user_id` represents the unique ID of the user that owns this
/// machine. `device_id` represents the unique ID of the device
/// The created machine will keep the encryption keys either in a IndexedDB
/// based store, or in a memory store and once the objects is dropped,
/// the keys will be lost.
///
/// # Arguments
///
/// * `user_id` - represents the unique ID of the user that owns this
/// machine.
///
/// * `device_id` - represents the unique ID of the device
/// that owns this machine.
///
/// `store_name` and `store_passphrase` are both optional, but
/// must be both set to have an effect. If they are both set, the
/// state of the machine will persist in a database named
/// `store_name` where its content is encrypted by the passphrase
/// given by `store_passphrase`. If they are not both set, the
/// created machine will keep the encryption keys only in memory,
/// and once the object is dropped, the keys will be lost.
#[wasm_bindgen(constructor)]
#[allow(clippy::new_ret_no_self)]
pubfnnew(
/// * `store_name` - The name that should be used to open the IndexedDB
/// based database. If this isn't provided, a memory-only store will be
/// used. *Note* the memory-only store will lose your E2EE keys when the
/// `OlmMachine` gets dropped.
///
/// * `store_passphrase` - The passphrase that should be used to encrypt the
/// IndexedDB based
pubfninitialize(
user_id: &identifiers::UserId,
device_id: &identifiers::DeviceId,
store_name: Option<String>,
@@ -57,15 +75,7 @@ impl OlmMachine {
future_to_promise(asyncmove{
letstore=match(store_name,store_passphrase){
// We need this `#[cfg]` because `IndexeddbCryptoStore`
// implements `CryptoStore` only on `target_arch =
// "wasm32"`. Without that, we could have a compilation
// error when checking the entire workspace. In
// practise, it doesn't impact this crate because it's
// always compiled for `wasm32`.
#[cfg(target_arch = "wasm32")]
(Some(store_name),Some(mutstore_passphrase))=>{
usestd::sync::Arc;
usezeroize::Zeroize;
letstore=Some(
@@ -73,8 +83,7 @@ impl OlmMachine {
&store_name,
&store_passphrase,
)
.await
.map(Arc::new)?,
.await?,
);
store_passphrase.zeroize();
@@ -82,15 +91,29 @@ impl OlmMachine {
store
}
(Some(_),None)=>returnErr(anyhow::Error::msg("The `store_name` has been set, and so, it expects a `store_passphrase`, which is not set; please provide one")),
(None,Some(_))=>returnErr(anyhow::Error::msg("The `store_passphrase` has been set, but it has an effect only if `store_name` is set, which is not; please provide one")),
(None,Some(_))=>{
returnErr(anyhow::Error::msg(
"The `store_passphrase` has been set, but it has an effect only if \
`store_name` is set, which is not; please provide one",
))
}
_=>None,
(None,None)=>None,
};
Ok(OlmMachine{
inner: matchstore{
// We need this `#[cfg]` because `IndexeddbCryptoStore`
// implements `CryptoStore` only on `target_arch =
// "wasm32"`. Without that, we could have a compilation
// error when checking the entire workspace. In practice,
// it doesn't impact this crate because it's always
Some files were not shown because too many files have changed in this diff
Show More
Reference in New Issue
Block a user
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Richard van der Hoff