mirror/matrix-rust-sdk
1
0
Fork 0
mirror of https://github.com/matrix-org/matrix-rust-sdk.git synced 2026-05-10 08:53:00 -04:00
Code Issues Packages Projects Releases Wiki Activity

fix(sliding sync): don't mark a response to a locally empty room as limited

Browse Source 
When receiving a sliding sync room response for a room that had no local events in its timeline cache,
we'd mark the room as limited before, which is incorrect. It was made worse by the fact that later in
the code, we'd clear the local cache if a room was marked as limited, so this is a problem that would
repeat itself over time (assuming empty responses for that room).

This fixes it and unifies logs so there's only one log line per room, at most.

Fixes https://github.com/vector-im/element-x-android/issues/1281
Fixes https://github.com/matrix-org/matrix-rust-sdk/issues/2540
This commit is contained in:
Benjamin Bouvier
2023-09-12 15:32:18 +02:00
parent a53bfe5748
commit 654a2f2495
1 changed files with 62 additions and 49 deletions
Download Patch File Download Diff File Expand all files Collapse all files

111
crates/matrix-sdk/src/sliding_sync/mod.rs
View File

@@ -945,72 +945,65 @@ impl StickyData for SlidingSyncStickyParameters {
// NOTE: SS proxy workaround.
#[instrument(skip_all)]
fn compute_limited(
known_rooms: &BTreeMap<OwnedRoomId, SlidingSyncRoom>,
response_rooms: &mut BTreeMap<OwnedRoomId, v4::SlidingSyncRoom>,
local_rooms: &BTreeMap<OwnedRoomId, SlidingSyncRoom>,
remote_rooms: &mut BTreeMap<OwnedRoomId, v4::SlidingSyncRoom>,
) {
for (room_id, room) in response_rooms {
for (room_id, remote_room) in remote_rooms {
// Only rooms marked as initially loaded are subject to the fixup.
let initial = room.initial.unwrap_or(false);
let initial = remote_room.initial.unwrap_or(false);
if !initial {
continue;
}
if room.limited {
if remote_room.limited {
// If the room was already marked as limited, the server knew more than we do.
continue;
}
trace!(?room_id, "starting");
let remote_events = &remote_room.timeline;
if remote_events.is_empty() {
trace!("no timeline updates in the response => not limited");
continue;
}
// If the known room had some timeline events, consider it's a `limited` if
// there's absolutely no overlap between the known events and
// the new events in the timeline.
if let Some(known_room) = known_rooms.get(room_id) {
let known_events = known_room.timeline_queue();
if let Some(known_room) = local_rooms.get(room_id) {
let local_events = known_room.timeline_queue();
if room.timeline.is_empty() && !known_events.is_empty() {
// If the cached timeline had events, but the one in the response didn't have
// any, don't mark the room as limited.
trace!(
"no timeline updates in response, local timeline had events => not limited."
);
if local_events.is_empty() {
trace!("local timeline had no events => not limited");
continue;
}
// Gather all the known event IDs. Ignore events that don't have an event ID.
let num_known_events = known_events.len();
let known_events: HashSet<OwnedEventId> =
HashSet::from_iter(known_events.into_iter().filter_map(|event| event.event_id()));
if num_known_events != known_events.len() {
trace!(
"{} local timeline events had no IDs",
num_known_events - known_events.len()
);
}
let num_local_events = local_events.len();
let local_events_with_ids: HashSet<OwnedEventId> =
HashSet::from_iter(local_events.into_iter().filter_map(|event| event.event_id()));
// There's overlap if, and only if, there's at least one event in the
// response's timeline that matches an event id we've seen before.
let mut num_missing_event_ids = 0;
let overlap = room.timeline.iter().any(|seen_event| {
if let Some(seen_event_id) =
seen_event.get_field::<OwnedEventId>("event_id").ok().flatten()
let mut num_remote_events_missing_ids = 0;
let overlap = remote_events.iter().any(|remote_event| {
if let Some(remote_event_id) =
remote_event.get_field::<OwnedEventId>("event_id").ok().flatten()
{
known_events.contains(&seen_event_id)
local_events_with_ids.contains(&remote_event_id)
} else {
warn!("unable to get event_id from {seen_event:?} when computing limited flag");
num_missing_event_ids += 1;
num_remote_events_missing_ids += 1;
false
}
});
room.limited = !overlap;
remote_room.limited = !overlap;
trace!(
num_events_response = room.timeline.len(),
num_events_local = known_events.len(),
num_missing_event_ids,
room_limited = room.limited,
num_events_response = remote_events.len(),
num_local_events,
num_local_events_with_ids = local_events_with_ids.len(),
num_remote_events_missing_ids,
room_limited = remote_room.limited,
"done"
);
} else {
@@ -1935,12 +1928,13 @@ mod tests {
let no_overlap = room_id!("!omelette:example.org");
let partial_overlap = room_id!("!fromage:example.org");
let complete_overlap = room_id!("!baguette:example.org");
let no_new_events = room_id!("!pain:example.org");
let no_remote_events = room_id!("!pain:example.org");
let no_local_events = room_id!("!crepe:example.org");
let already_limited = room_id!("!paris:example.org");
let response_timeline = vec![event_c.event.clone(), event_d.event.clone()];
let known_rooms = BTreeMap::from_iter([
let local_rooms = BTreeMap::from_iter([
(
// This has no events overlapping with the response timeline, hence limited, but
// it's not marked as initial in the response.
@@ -1985,14 +1979,25 @@ mod tests {
(
// We locally have events for this room, and receive none in the response: not
// limited.
no_new_events.to_owned(),
no_remote_events.to_owned(),
SlidingSyncRoom::new(
client.clone(),
no_new_events.to_owned(),
no_remote_events.to_owned(),
v4::SlidingSyncRoom::default(),
vec![event_c.clone(), event_d.clone()],
),
),
(
// We don't have events for this room locally, and even if the remote room contains
// some events, it's not a limited sync.
no_local_events.to_owned(),
SlidingSyncRoom::new(
client.clone(),
no_local_events.to_owned(),
v4::SlidingSyncRoom::default(),
vec![],
),
),
(
// Already limited, but would be marked limited if the flag wasn't ignored (same as
// partial overlap).
@@ -2006,7 +2011,7 @@ mod tests {
),
]);
let mut response_rooms = BTreeMap::from_iter([
let mut remote_rooms = BTreeMap::from_iter([
(
not_initial.to_owned(),
assign!(v4::SlidingSyncRoom::default(), { timeline: response_timeline }),
@@ -2033,12 +2038,19 @@ mod tests {
}),
),
(
no_new_events.to_owned(),
no_remote_events.to_owned(),
assign!(v4::SlidingSyncRoom::default(), {
initial: Some(true),
timeline: vec![],
}),
),
(
no_local_events.to_owned(),
assign!(v4::SlidingSyncRoom::default(), {
initial: Some(true),
timeline: vec![event_c.event.clone(), event_d.event.clone()],
}),
),
(
already_limited.to_owned(),
assign!(v4::SlidingSyncRoom::default(), {
@@ -2049,14 +2061,15 @@ mod tests {
),
]);
compute_limited(&known_rooms, &mut response_rooms);
compute_limited(&local_rooms, &mut remote_rooms);
assert!(!response_rooms.get(not_initial).unwrap().limited);
assert!(response_rooms.get(no_overlap).unwrap().limited);
assert!(!response_rooms.get(partial_overlap).unwrap().limited);
assert!(!response_rooms.get(complete_overlap).unwrap().limited);
assert!(!response_rooms.get(no_new_events).unwrap().limited);
assert!(response_rooms.get(already_limited).unwrap().limited);
assert!(!remote_rooms.get(not_initial).unwrap().limited);
assert!(remote_rooms.get(no_overlap).unwrap().limited);
assert!(!remote_rooms.get(partial_overlap).unwrap().limited);
assert!(!remote_rooms.get(complete_overlap).unwrap().limited);
assert!(!remote_rooms.get(no_remote_events).unwrap().limited);
assert!(!remote_rooms.get(no_local_events).unwrap().limited);
assert!(remote_rooms.get(already_limited).unwrap().limited);
}
#[async_test]