feat: add fn to await a synced room from Client
#3979
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3979 +/- ##
==========================================
+ Coverage 84.27% 84.32% +0.05%
==========================================
Files 266 266
Lines 28286 28296 +10
==========================================
+ Hits 23838 23862 +24
+ Misses 4448 4434 -14 ☔ View full report in Codecov by Sentry. |
crates/matrix-sdk/src/client/mod.rs
Outdated
| return room; | ||
| } else { | ||
| warn!("Room wasn't partially synced, waiting for sync beat to try again"); | ||
| } |
There was a problem hiding this comment.
nit: no else after return please :)
Also I think the warn can just be a debug, it's not high priority enough that it should jump out to our eyes when looking at rageshakes IMO.
crates/matrix-sdk/src/client/mod.rs
Outdated
| warn!("Room wasn't partially synced, waiting for sync beat to try again"); | ||
| } | ||
| } else { | ||
| warn!("Room wasn't found, waiting for sync beat to try again"); |
crates/matrix-sdk/src/client/mod.rs
Outdated
| warn!("Room wasn't found, waiting for sync beat to try again"); | ||
| } | ||
| self.inner.sync_beat.listen().await; | ||
| debug!("New sync beat found"); |
There was a problem hiding this comment.
and this one we can remove, since the next loop iteration will immediately log something or return.
crates/matrix-sdk/src/client/mod.rs
Outdated
| let room = | ||
| tokio::time::timeout(Duration::from_secs(1), client.await_room_remote_echo(room_id)) | ||
| .await | ||
| .unwrap(); |
There was a problem hiding this comment.
We can do slightly better here, and not involve time at all:
let room = client.await_room_remote_echo(room_id).now_or_never().unwrap();(now_or_never() requires a trait to be in scope)
crates/matrix-sdk/src/client/mod.rs
Outdated
|
|
||
| // Perform the /sync request with a delay so it starts after the | ||
| // `await_room_remote_echo` call has happened | ||
| tokio::spawn({ |
There was a problem hiding this comment.
nit, here and below for the tokio:: usage, please add use tokio:: statements at the top of this file, so it's shorter to read here
| /// **Note: this function will loop endlessly until either it finds the room | ||
| /// or an externally set timeout happens.** |
There was a problem hiding this comment.
i'd do a slightly different API at the FFI level: add an Option<Duration> as a parameter, and add a timeout here, so the consumer doesn't have to. Does it make sense? (You get to decide if it's a good idea, based on your usage of the API in the app :))
There was a problem hiding this comment.
I asked @stefanceriu and we decided to have the client implement the timeout.
| .await | ||
| .map_err(Error::EventCache)?; | ||
| .map_err(Error::EventCache)?; | ||
| } |
There was a problem hiding this comment.
Can you add some info! statement in the else case, to indicate that we didn't have a timeline cache for this room? That will help debunk issues when people are in offline mode.
|
My bad, I have addressed the rest in 606c6b9, I thought I had already uploaded it. |
This new fn is used to check when a room is at least partially synced, which seems to be the case with SSS.
This fn will loop until it finds an at least partially synced room with the given id. It uses the `ClientInner::sync_beat` listener to wait until the next check is needed.
…oom_timeline_builder`. Having initial items shouldn't be mandatory to create a timeline, the timeline can also be empty.
jmartinesp
force-pushed
the
feat/allow-client-to-wait-for-a-room-remote-echo
branch
from
606c6b9
to
2693857
Compare
jmartinesp
deleted the
feat/allow-client-to-wait-for-a-room-remote-echo
branch
Changes
fn sdk_base::Room::is_state_partially_or_fully_synced(). I was planning to reuseRoom::is_state_fully_synced(), but this apparently doesn't work with SSS.fn Client::await_room_remote_echo(&RoomId), which will loop indefinitely until it finds a room with the provided room id, usingClientInner::sync_beatto delay the checks. This is the core of the changes, and an FFI fn has been created to access it.SlidingSyncRoomis no longer needed infn RoomListItem::default_room_timeline_builder(). It was mandatory, and used only to pre-fill the timeline with initial items. After discussing this behavior we decided we can make it non-mandatory so timelines can be created for rooms that haven't been synced yet, which was a source of pain in the clients.TimelineState.Motivation
After using
Client::create_roomin the clients we found some cases where we were trying to create a timeline for the just created room while it still hadn't been synced and this resulted in an error and the full room not being returned to the clients. The newawait_room_remote_echofn will let us wait until the room has been synced, fixing this issue.Also, there may be some cases where a room is in the local DB but it hasn't been synced yet. The behaviour change in
default_room_timeline_buildershould fix that.Signed-off-by: