event cache/timeline: reuse the Paginator when running back-paginations
#3373
Conversation
bnjbvr
requested review from
andybalaam and
Hywan
and removed request for
a team and
andybalaam
…ationData` as well
…ound Only code motion. No changes in functionality.
No changes in functionality, just renamings.
…agination that failed under our feet When a timeline reset happens while we're back-paginating, the event cache method to run back pagination would return an success result indicating that the pagination token disappeared. After thinking about it, it's not the best API in the world; ideally, the backpagination mechanism would restart automatically. Now, this was handled in the timeline before, and the reason it was handled there was because it was possible to back-paginate and ask for a certain number of events. I've removed that feature, so that back-pagination on a live timeline matches the capabilities of a focused-timeline back-pagination: one can only ask for a given number of *events*, not timeline items. As a matter of fact, this simplifies the code a lot by removing many data structures, that were also exposed (and unused, since recent changes) in the FFI layer.
bnjbvr
force-pushed
the
bnjbvr/reuse-paginator-in-event-cache
branch
from
20404fb
to
bbd20b0
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3373 +/- ##
==========================================
- Coverage 82.96% 82.94% -0.03%
==========================================
Files 245 246 +1
Lines 24901 24884 -17
==========================================
- Hits 20660 20640 -20
- Misses 4241 4244 +3 ☔ View full report in Codecov by Sentry. |
| pub fn back_pagination_status(&self) -> Subscriber<PaginationStatus> { | ||
| self.back_pagination_status.subscribe() | ||
| /// | ||
| /// Note: this may send multiple Paginating/Idle sequences during a single |
There was a problem hiding this comment.
Why? Do we want to dedup them? Maybe with async_rx::StreamExt::dedup if we can provide a Stream.
There was a problem hiding this comment.
It's an implementation detail of eyeball that we're running into. If you have an emitter E and an observer O. Say E emits very quickly e1, e2, e3; then if O doesn't react fast enough, it may only see e1, handle it, and then miss e2, and only see e3.
Before this PR, the exposed pagination status was controlled at the timeline level, and the timeline would make sure to reset it only when a full pagination loop is over, after we obtained e.g. N events.
After this PR, the pagination status is that of the underlying paginator. The timeline may continue to paginate after the paginator has run one pagination query. In that case, the underlying status of the paginator will be:
- running (because it's busy doing the network request)
- idle (because it's done processing the response)
- (the timeline sees it hasn't received as many events as it wanted, so it retriggers the paginator) the paginator quickly re-runs into the running state
So the quick running -> idle -> running transition might make it so that an observer could see running -> running, because they didn't react fast enough to see the idle state in the middle.
I've added a dedup() call to address this, thanks.
Now, if the caller is actually fast enough, they may see a few quick transitions from running -> idle -> running; I wonder if that may cause some UI flickering (e.g. the spinner quickly disappears and reappears). At this point, both EX apps doesn't use the back-pagination status anymore, so it's a theoretical issue.
| pagination_token_notifier: Default::default(), | ||
| } | ||
| } | ||
|
|
||
| async fn clear(&self, room_events: &mut RwLockWriteGuard<'_, RoomEvents>) { |
There was a problem hiding this comment.
Why passing the room_events? What's the benefits of this new approach?
There was a problem hiding this comment.
Initially, I took the room_events lock within this function; but when this is called in replace_all_events_by, we're already holding onto the lock, and we really want to keep it throughout the full function's body, to avoid a bad race when two callers racily call replace_all_events_by.
|
This is ready for another round of review, please! (Note: this will need to be squashed, as the commit history has gotten a bit messy, because of large conflicts with linked chunks) |
First commit
Sorry, I got a bit ahead of myself and did multiple interesting changes in that large commit:
Paginatormust now be instantiable from that state, with a previous-batch token that's passed by the caller. This justified introducingPaginator::set_idle_state(prev_batch_token).prev_batch_tokenset toNoneto that function (meaning, we don't have any prev-batch-token from sync for that room), we now need to distinguishprev_batch_token is set to None by choicefromprev_batch_token is set to None because we hit the end of the timeline. This justified the introduction ofpaginator::PaginationToken, which reflects those three states (the third one beingSome(token)).Paginatorfor the room back-paginations. TheRoomEventCachenow also takes care of the pagination token, instead of being handed one, removing some failure modes (e.g. passing an incorrect back-pagination token internally). If it didn't get a proper token from a sync, it'll be in charge of waiting for it (for 3 seconds), so the timeline doesn't have to do that itself anymore.Other commits
The other commits are slightly more atomic and should be easier to digest one by one, by looking at the commit messages.
TL;DR
This makes it possible for the event cache to back-paginate in the background, even when no timelines are shown, which should be handy for some use cases (esp. around read-receipts).
Fixes #3355.