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.
