feat: Only restart growing the allRooms list in case of errors #2208
Conversation
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).
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #2208 +/- ##
=======================================
Coverage 76.71% 76.72%
=======================================
Files 164 164
Lines 17646 17648 +2
=======================================
+ Hits 13538 13540 +2
Misses 4108 4108
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
No longer refreshing the all_rooms list when the sync-loop has been terminated is impactful. ElementX for example calls RoomListService::stop_sync() every time the app is backgrounded for 30sec. So once the app is foregrounded, it will continue from where it left. If the app has been backgrounded for, let say 10mn, an important volume of data might be fetched (with a fully loaded room list of 4000 rooms for example).
One may argue that 10mn is enought to make the sliding sync session to expire. In this case, well, yeah, sliding sync will receive an M_UNKNOWN_POS error, which will refresh the list.
Otherwise, if the metrics shows that we are OK with that, why not.
Maybe it's time to share an idea I had since a couple of days:
- In case of an error: refresh the list with the default batch size, be 50,
- In case of a termination: refresh the list with a larger batch size, like 200 or even 500.
Would it help or would it not solve the current problem with the sliding sync proxy server?
| // Refresh the lists only if our sync ran into an error (in particular, | ||
| // when the session was invalidated by the server). Otherwise, keep on | ||
| // iterating on the previous list. | ||
| if matches!(self, Error { .. }) { |
There was a problem hiding this comment.
I think the big match can be simplified as is:
let (next_state, actions) = match self {
Init => (SettingUp, Actions::none()),
SettingUp => (Running, Actions::first_rooms_are_loaded()),
Running => (Running, Actions::none()),
Terminated { from: previous_state } => match previous_state.as_ref() {
Error { .. } | Terminated { .. } => {
unreachable!("…");
}
state => {
// Do nothing.
(state.to_owned(), Actions::none())
}
},
Error { from: previous_state } => match previous_state.as_ref() {
Error { .. } | Terminated { .. } => {
unreachable!("…");
}
Running => {
// Refresh the lists.
(Running, Actions::refresh_lists())
}
state => {
// Do nothing.
(state.to_owned(), Actions::none())
}
},
};Thoughts?
Edit @Hywan: Address #1911.
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 the delivery of live data. As a matter of fact, to get a quicker response from 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).