Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(sdk,ui): SlidingSync::subscribe_to_rooms has a new cancel_in_flight_request argument #3981

Merged
merged 1 commit into from
Sep 16, 2024
Merged

feat(sdk,ui): SlidingSync::subscribe_to_rooms has a new cancel_in_flight_request argument #3981

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions crates/matrix-sdk-ui/src/notification_client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,7 @@ impl NotificationClient {
required_state,
timeline_limit: Some(uint!(16))
})),
true,
);

let mut remaining_attempts = 3;
Expand Down
9 changes: 8 additions & 1 deletion crates/matrix-sdk-ui/src/room_list_service/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,7 +395,14 @@ impl RoomListService {
settings.required_state.push((StateEventType::RoomCreate, "".to_owned()));
}

self.sliding_sync.subscribe_to_rooms(room_ids, Some(settings))
let cancel_in_flight_request = match self.state.get() {
State::Init | State::Recovering | State::Error { .. } | State::Terminated { .. } => {
false
}
State::SettingUp | State::Running => true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

turn this new argument to true if the state machine isn't in a “starting” state.

This seems to set it to true when we're in the Running state, are you sure this is correct? State::Init sounds more like it would fit the description.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is correct yes. Init and Recovering are the “starting” states, thus we don't want to cancel in-flight requests. SettingUp and Running are states where we have a pos, so we can cancel in-flight requests here.

The state machine is like this: Init -> SettingUp -> Running. In case of an error, it depends on the previous state before the error, but basically if we were in Running, we jump back to Recovering -> Running. So Init and Recovering are basically the “starting” states here.

};

self.sliding_sync.subscribe_to_rooms(room_ids, Some(settings), cancel_in_flight_request)
}

#[cfg(test)]
Expand Down
17 changes: 9 additions & 8 deletions crates/matrix-sdk/src/sliding_sync/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,12 +151,13 @@ impl SlidingSync {
&self,
room_ids: &[&RoomId],
settings: Option<http::request::RoomSubscription>,
cancel_in_flight_request: bool,
) {
let settings = settings.unwrap_or_default();
let mut sticky = self.inner.sticky.write().unwrap();
let room_subscriptions = &mut sticky.data_mut().room_subscriptions;

let mut skip_sync_loop = false;
let mut skip_over_current_sync_loop_iteration = false;

for room_id in room_ids {
// If the room subscription already exists, let's not
Expand All @@ -172,11 +173,11 @@ impl SlidingSync {

entry.insert((RoomSubscriptionState::default(), settings.clone()));

skip_sync_loop = true;
skip_over_current_sync_loop_iteration = true;
}
}

if skip_sync_loop {
if cancel_in_flight_request && skip_over_current_sync_loop_iteration {
self.inner.internal_channel_send_if_possible(
SlidingSyncInternalMessage::SyncLoopSkipOverCurrentIteration,
);
Expand Down Expand Up @@ -1213,7 +1214,7 @@ mod tests {
// Members are now synced! We can start subscribing and see how it goes.
assert!(room0.are_members_synced());

sliding_sync.subscribe_to_rooms(&[room_id_0, room_id_1], None);
sliding_sync.subscribe_to_rooms(&[room_id_0, room_id_1], None, true);

// OK, we have subscribed to some rooms. Let's check on `room0` if members are
// now marked as not synced.
Expand Down Expand Up @@ -1254,7 +1255,7 @@ mod tests {
// Members are synced, good, good.
assert!(room0.are_members_synced());

sliding_sync.subscribe_to_rooms(&[room_id_0], None);
sliding_sync.subscribe_to_rooms(&[room_id_0], None, false);

// Members are still synced: because we have already subscribed to the
// room, the members aren't marked as unsynced.
Expand All @@ -1274,7 +1275,7 @@ mod tests {
let room_id_2 = room_id!("!r2:bar.org");

// Subscribe to two rooms.
sliding_sync.subscribe_to_rooms(&[room_id_0, room_id_1], None);
sliding_sync.subscribe_to_rooms(&[room_id_0, room_id_1], None, false);

{
let sticky = sliding_sync.inner.sticky.read().unwrap();
Expand All @@ -1286,7 +1287,7 @@ mod tests {
}

// Subscribe to one more room.
sliding_sync.subscribe_to_rooms(&[room_id_2], None);
sliding_sync.subscribe_to_rooms(&[room_id_2], None, false);

{
let sticky = sliding_sync.inner.sticky.read().unwrap();
Expand All @@ -1308,7 +1309,7 @@ mod tests {
}

// Subscribe to one room again.
sliding_sync.subscribe_to_rooms(&[room_id_2], None);
sliding_sync.subscribe_to_rooms(&[room_id_2], None, false);

{
let sticky = sliding_sync.inner.sticky.read().unwrap();
Expand Down
Loading