Skip to content

Commit

Permalink
fix(ui): Consider timeline_limit in sliding sync as non-sticky.
Browse files Browse the repository at this point in the history 
This patch changes the behaviour of `timeline_limit` in sliding sync
requests. It previously was sticky, but since it's now mandatory
with MSC4186, it's preferable it to be non-sticky, otherwise in
some scenarios it might default to 0 (its default value). How?
If the server doesn't reply with our `txn_id` (because it doesn't
support sticky parameters or because it misses a `txn_id`), the
next request will be built with a default `timeline_limit` value,
which is zero, and won't get updated to the `timeline_limit` value
from `SlidingSyncListStickyParameters`. This is not good. Instead,
we must consider `timeline_limit` as non-sticky, and moves it from
`SlidingSyncListStickyParameters` to `SlidingSyncListInner`. This is
what this patch does.
  • Loading branch information
@Hywan
Hywan committed Oct 7, 2024
1 parent 26f3632 commit 9807357
Show file tree
Hide file tree
Showing 4 changed files with 45 additions and 59 deletions.
66 changes: 33 additions & 33 deletions crates/matrix-sdk-ui/tests/integration/room_list_service.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,7 @@ async fn test_sync_all_states() -> Result<(), Error> {
"lists": {
ALL_ROOMS: {
"ranges": [[0, 99]],
"timeline_limit": 0,
"timeline_limit": 1,
},
},
},
Expand All @@ -401,7 +401,7 @@ async fn test_sync_all_states() -> Result<(), Error> {
"lists": {
ALL_ROOMS: {
"ranges": [[0, 199]],
"timeline_limit": 0,
"timeline_limit": 1,
},
},
},
Expand All @@ -426,7 +426,7 @@ async fn test_sync_all_states() -> Result<(), Error> {
"lists": {
ALL_ROOMS: {
"ranges": [[0, 299]],
"timeline_limit": 0,
"timeline_limit": 1,
},
},
},
Expand All @@ -451,7 +451,7 @@ async fn test_sync_all_states() -> Result<(), Error> {
"lists": {
ALL_ROOMS: {
"ranges": [[0, 399]],
"timeline_limit": 0,
"timeline_limit": 1,
},
},
},
Expand Down Expand Up @@ -515,7 +515,7 @@ async fn test_sync_resumes_from_previous_state() -> Result<(), Error> {
"lists": {
ALL_ROOMS: {
"ranges": [[0, 9]],
"timeline_limit": 0,
"timeline_limit": 1,
},
},
},
Expand Down Expand Up @@ -543,7 +543,7 @@ async fn test_sync_resumes_from_previous_state() -> Result<(), Error> {
"lists": {
ALL_ROOMS: {
"ranges": [[0, 9]],
"timeline_limit": 0,
"timeline_limit": 1,
},
},
},
Expand Down Expand Up @@ -627,7 +627,7 @@ async fn test_sync_resumes_from_error() -> Result<(), Error> {
ALL_ROOMS: {
// The sync-mode has changed to growing, with its initial range.
"ranges": [[0, 99]],
"timeline_limit": 0,
"timeline_limit": 1,
},
},
},
Expand All @@ -652,7 +652,7 @@ async fn test_sync_resumes_from_error() -> Result<(), Error> {
ALL_ROOMS: {
// Due to previous error, the sync-mode is back to selective, with its initial range.
"ranges": [[0, 19]],
"timeline_limit": 0,
"timeline_limit": 1,
},
},
},
Expand All @@ -675,7 +675,7 @@ async fn test_sync_resumes_from_error() -> Result<(), Error> {
ALL_ROOMS: {
// Sync-mode is now growing.
"ranges": [[0, 99]],
"timeline_limit": 0,
"timeline_limit": 1,
},
},
},
Expand All @@ -699,7 +699,7 @@ async fn test_sync_resumes_from_error() -> Result<(), Error> {
ALL_ROOMS: {
// The sync-mode is still growing, and the range has made progress.
"ranges": [[0, 199]],
"timeline_limit": 0,
"timeline_limit": 1,
},
},
},
Expand All @@ -724,7 +724,7 @@ async fn test_sync_resumes_from_error() -> Result<(), Error> {
ALL_ROOMS: {
// Due to previous error, the sync-mode is back to selective, with its initial range.
"ranges": [[0, 19]],
"timeline_limit": 0,
"timeline_limit": 1,
},
},
},
Expand All @@ -747,7 +747,7 @@ async fn test_sync_resumes_from_error() -> Result<(), Error> {
ALL_ROOMS: {
// The sync-mode is now growing.
"ranges": [[0, 99]],
"timeline_limit": 0,
"timeline_limit": 1,
},
},
},
Expand All @@ -770,7 +770,7 @@ async fn test_sync_resumes_from_error() -> Result<(), Error> {
ALL_ROOMS: {
// No error. The range is making progress.
"ranges": [[0, 199]],
"timeline_limit": 0,
"timeline_limit": 1,
},
},
},
Expand All @@ -795,7 +795,7 @@ async fn test_sync_resumes_from_error() -> Result<(), Error> {
// Range is making progress and is even reaching the maximum
// number of rooms.
"ranges": [[0, 209]],
"timeline_limit": 0,
"timeline_limit": 1,
},
},
},
Expand All @@ -820,7 +820,7 @@ async fn test_sync_resumes_from_error() -> Result<(), Error> {
ALL_ROOMS: {
// Due to previous error, the sync-mode is back to selective, with its initial range.
"ranges": [[0, 19]],
"timeline_limit": 0,
"timeline_limit": 1,
},
},
},
Expand All @@ -843,7 +843,7 @@ async fn test_sync_resumes_from_error() -> Result<(), Error> {
ALL_ROOMS: {
// Sync-mode is now growing.
"ranges": [[0, 99]],
"timeline_limit": 0,
"timeline_limit": 1,
},
},
},
Expand Down Expand Up @@ -913,7 +913,7 @@ async fn test_sync_resumes_from_terminated() -> Result<(), Error> {
ALL_ROOMS: {
// The sync-mode is still selective, with its initial range.
"ranges": [[0, 19]],
"timeline_limit": 0,
"timeline_limit": 1,
},
},
},
Expand All @@ -937,7 +937,7 @@ async fn test_sync_resumes_from_terminated() -> Result<(), Error> {
ALL_ROOMS: {
// The sync-mode is now growing, with its initial range.
"ranges": [[0, 99]],
"timeline_limit": 0,
"timeline_limit": 1,
},
},
},
Expand Down Expand Up @@ -969,7 +969,7 @@ async fn test_sync_resumes_from_terminated() -> Result<(), Error> {
ALL_ROOMS: {
// The sync-mode is back to selective.
"ranges": [[0, 19]],
"timeline_limit": 0,
"timeline_limit": 1,
},
},
},
Expand All @@ -993,7 +993,7 @@ async fn test_sync_resumes_from_terminated() -> Result<(), Error> {
ALL_ROOMS: {
// Sync-mode is growing, with its initial range.
"ranges": [[0, 99]],
"timeline_limit": 0,
"timeline_limit": 1,
},
},
},
Expand All @@ -1017,7 +1017,7 @@ async fn test_sync_resumes_from_terminated() -> Result<(), Error> {
ALL_ROOMS: {
// Range is making progress, and has reached its maximum.
"ranges": [[0, 149]],
"timeline_limit": 0,
"timeline_limit": 1,
},
},
},
Expand Down Expand Up @@ -1089,7 +1089,7 @@ async fn test_loading_states() -> Result<(), Error> {
"lists": {
ALL_ROOMS: {
"ranges": [[0, 9]],
"timeline_limit": 0,
"timeline_limit": 1,
},
},
},
Expand Down Expand Up @@ -1120,7 +1120,7 @@ async fn test_loading_states() -> Result<(), Error> {
"lists": {
ALL_ROOMS: {
"ranges": [[0, 11]],
"timeline_limit": 0,
"timeline_limit": 1,
},
},
},
Expand Down Expand Up @@ -1271,7 +1271,7 @@ async fn test_dynamic_entries_stream() -> Result<(), Error> {
"lists": {
ALL_ROOMS: {
"ranges": [[0, 9]],
"timeline_limit": 0,
"timeline_limit": 1,
},
},
},
Expand Down Expand Up @@ -1381,7 +1381,7 @@ async fn test_dynamic_entries_stream() -> Result<(), Error> {
"lists": {
ALL_ROOMS: {
"ranges": [[0, 9]],
"timeline_limit": 0,
"timeline_limit": 1,
},
},
},
Expand Down Expand Up @@ -1548,7 +1548,7 @@ async fn test_dynamic_entries_stream() -> Result<(), Error> {
"lists": {
ALL_ROOMS: {
"ranges": [[0, 9]],
"timeline_limit": 0,
"timeline_limit": 1,
},
},
},
Expand Down Expand Up @@ -1718,7 +1718,7 @@ async fn test_room_sorting() -> Result<(), Error> {
"lists": {
ALL_ROOMS: {
"ranges": [[0, 4]],
"timeline_limit": 0,
"timeline_limit": 1,
},
},
},
Expand Down Expand Up @@ -1804,7 +1804,7 @@ async fn test_room_sorting() -> Result<(), Error> {
"lists": {
ALL_ROOMS: {
"ranges": [[0, 4]],
"timeline_limit": 0,
"timeline_limit": 1,
},
},
},
Expand Down Expand Up @@ -1879,7 +1879,7 @@ async fn test_room_sorting() -> Result<(), Error> {
"lists": {
ALL_ROOMS: {
"ranges": [[0, 5]],
"timeline_limit": 0,
"timeline_limit": 1,
},
},
},
Expand Down Expand Up @@ -2099,7 +2099,7 @@ async fn test_room_subscription() -> Result<(), Error> {
"lists": {
ALL_ROOMS: {
"ranges": [[0, 2]],
"timeline_limit": 0,
"timeline_limit": 1,
},
},
"room_subscriptions": {
Expand Down Expand Up @@ -2143,7 +2143,7 @@ async fn test_room_subscription() -> Result<(), Error> {
"lists": {
ALL_ROOMS: {
"ranges": [[0, 2]],
"timeline_limit": 0,
"timeline_limit": 1,
},
},
"room_subscriptions": {
Expand Down Expand Up @@ -2190,7 +2190,7 @@ async fn test_room_subscription() -> Result<(), Error> {
"lists": {
ALL_ROOMS: {
"ranges": [[0, 2]],
"timeline_limit": 0,
"timeline_limit": 1,
},
},
// NO `room_subscriptions`!
Expand Down Expand Up @@ -2257,7 +2257,7 @@ async fn test_room_unread_notifications() -> Result<(), Error> {
"lists": {
ALL_ROOMS: {
"ranges": [[0, 0]],
"timeline_limit": 0,
"timeline_limit": 1,
},
},
},
Expand Down
2 changes: 1 addition & 1 deletion crates/matrix-sdk/src/sliding_sync/list/builder.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -177,9 +177,9 @@ impl SlidingSyncListBuilder {
self.required_state,
self.include_heroes,
self.filters,
self.timeline_limit,
),
)),
timeline_limit: StdRwLock::new(self.timeline_limit),
name: self.name,
cache_policy: self.cache_policy,

Expand Down
18 changes: 10 additions & 8 deletions crates/matrix-sdk/src/sliding_sync/list/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,12 +133,12 @@ impl SlidingSyncList {

/// Get the timeline limit.
pub fn timeline_limit(&self) -> Bound {
self.inner.sticky.read().unwrap().data().timeline_limit()
*self.inner.timeline_limit.read().unwrap()
}

/// Set timeline limit.
pub fn set_timeline_limit(&self, timeline: Bound) {
self.inner.sticky.write().unwrap().data_mut().set_timeline_limit(timeline);
*self.inner.timeline_limit.write().unwrap() = timeline;
}

/// Get the maximum number of rooms. See [`Self::maximum_number_of_rooms`]
Expand Down Expand Up @@ -233,6 +233,9 @@ pub(super) struct SlidingSyncListInner {
/// knows).
sticky: StdRwLock<SlidingSyncStickyManager<SlidingSyncListStickyParameters>>,

/// The maximum number of timeline events to query for.
timeline_limit: StdRwLock<Bound>,

/// The total number of rooms that is possible to interact with for the
/// given list.
///
Expand Down Expand Up @@ -307,12 +310,11 @@ impl SlidingSyncListInner {
/// request generator.
#[instrument(skip(self), fields(name = self.name))]
fn request(&self, ranges: Ranges, txn_id: &mut LazyTransactionId) -> http::request::List {
use ruma::UInt;

let ranges =
ranges.into_iter().map(|r| (UInt::from(*r.start()), UInt::from(*r.end()))).collect();
let ranges = ranges.into_iter().map(|r| ((*r.start()).into(), (*r.end()).into())).collect();

let mut request = assign!(http::request::List::default(), { ranges });
request.room_details.timeline_limit = (*self.timeline_limit.read().unwrap()).into();

{
let mut sticky = self.sticky.write().unwrap();
sticky.maybe_apply(&mut request, txn_id);
Expand Down Expand Up @@ -594,10 +596,10 @@ mod tests {
.timeline_limit(7)
.build(sender);

assert_eq!(list.inner.sticky.read().unwrap().data().timeline_limit(), 7);
assert_eq!(list.timeline_limit(), 7);

list.set_timeline_limit(42);
assert_eq!(list.inner.sticky.read().unwrap().data().timeline_limit(), 42);
assert_eq!(list.timeline_limit(), 42);
}

macro_rules! assert_ranges {
Expand Down
Loading

0 comments on commit 9807357

Please sign in to comment.