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

Refactor and clean up the SyncService #4543

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
+276 −205
Open

Refactor and clean up the SyncService #4543

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
e5763d6
refactor(ui): Move common data of the SyncService under a lock
poljar Jan 16, 2025
a735180
refactor(ui): Rename the scheduler task to supervisor task
poljar Jan 16, 2025
d00e609
refactor(ui): Create a Supervisor for the SyncService
poljar Jan 16, 2025
31cd71f
refactor(ui): Move the spawning of the child tasks into the supervisor
poljar Jan 16, 2025
8231688
refactor(ui): Move the expiration of the sync services closer to the …
poljar Jan 16, 2025
564a1f1
refactor(ui): Move the task spawning functions under the supervisor
poljar Jan 16, 2025
c1adf90
refactor(ui): Remove some early returns from the sync service
poljar Jan 16, 2025
f6b759c
refactor(ui): Prettify the two sync tasks a bit
poljar Jan 16, 2025
2261c6d
fix(ui): Shutdown the child tasks if the channel got closed in the su…
poljar Jan 16, 2025
5f62220
docs(ui): Polish the documentation of the SyncService a bit
poljar Jan 16, 2025
9ae0235
fixup! refactor(ui): Move common data of the SyncService under a lock
poljar Jan 17, 2025
b38e8b9
fixup! refactor(ui): Prettify the two sync tasks a bit
poljar Jan 17, 2025
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
47 changes: 24 additions & 23 deletions crates/matrix-sdk-ui/src/sync_service.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,33 +183,38 @@ impl SyncTaskSupervisor {
(room_list_task, encryption_sync_task)
}

fn check_if_expired(err: &matrix_sdk::Error) -> bool {
err.client_api_error_kind() == Some(&ruma::api::client::error::ErrorKind::UnknownPos)
}

async fn encryption_sync_task(
encryption_sync: Arc<EncryptionSyncService>,
sender: Sender<TerminationReport>,
sync_permit_guard: OwnedMutexGuard<EncryptionSyncPermit>,
) {
use encryption_sync_service::Error;

let encryption_sync_stream = encryption_sync.sync(sync_permit_guard);
pin_mut!(encryption_sync_stream);

let (is_error, has_expired) = loop {
let res = encryption_sync_stream.next().await;
match res {
match encryption_sync_stream.next().await {
Some(Ok(())) => {
// Carry on.
}
Some(Err(err)) => {
// If the encryption sync error was an expired session, also expire the
// room list sync.
let has_expired = if let encryption_sync_service::Error::SlidingSync(err) = &err
{
err.client_api_error_kind()
== Some(&ruma::api::client::error::ErrorKind::UnknownPos)
let has_expired = if let Error::SlidingSync(err) = &err {
Self::check_if_expired(err)
} else {
false
};

if !has_expired {
error!("Error while processing encryption in sync service: {err:#}");
}

break (true, has_expired);
}
None => {
Expand All @@ -219,14 +224,9 @@ impl SyncTaskSupervisor {
}
};

if let Err(err) = sender
.send(TerminationReport {
is_error,
has_expired,
origin: TerminationOrigin::EncryptionSync,
})
.await
{
let report =
TerminationReport { is_error, has_expired, origin: TerminationOrigin::EncryptionSync };
Copy link
Member

Choose a reason for hiding this comment

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

I don't see the point of the report variables: they only add one level of "indirection" (some extra thing to keep in mind for a reader), they create one extra binding for the compiler frontend to handle, and they're used only once, so might as well be kept inlined at use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer the formatting of the two statements compared to the formatting of the single statement, but ok: b38e8b9.

if let Err(err) = sender.send(report).await {
error!("Error while sending termination report: {err:#}");
}
}
Expand All @@ -235,27 +235,29 @@ impl SyncTaskSupervisor {
room_list_service: Arc<RoomListService>,
sender: Sender<TerminationReport>,
) {
use room_list_service::Error;

let room_list_stream = room_list_service.sync();
pin_mut!(room_list_stream);

let (is_error, has_expired) = loop {
let res = room_list_stream.next().await;
match res {
match room_list_stream.next().await {
Some(Ok(())) => {
// Carry on.
}
Some(Err(err)) => {
// If the room list error was an expired session, also expire the
// encryption sync.
let has_expired = if let room_list_service::Error::SlidingSync(err) = &err {
err.client_api_error_kind()
== Some(&ruma::api::client::error::ErrorKind::UnknownPos)
let has_expired = if let Error::SlidingSync(err) = &err {
Self::check_if_expired(err)
} else {
false
};

if !has_expired {
error!("Error while processing room list in sync service: {err:#}");
}

break (true, has_expired);
}
None => {
Expand All @@ -265,10 +267,9 @@ impl SyncTaskSupervisor {
}
};

if let Err(err) = sender
.send(TerminationReport { is_error, has_expired, origin: TerminationOrigin::RoomList })
.await
{
let report =
TerminationReport { is_error, has_expired, origin: TerminationOrigin::RoomList };
if let Err(err) = sender.send(report).await {
error!("Error while sending termination report: {err:#}");
}
}
Expand Down