Refactor and clean up the SyncService #4543

poljar · 2025-01-16T16:27:02Z

These are mostly internal changes and don't contain any behavioral change, except for some edge cases that are now a bit better handled.

A review commit by commit should ease the task a bit, but reading the whole file shouldn't be too terrible either, it's not that big.

bnjbvr

Haven't looked at all of it, but some of the cosmetic changes seem plain wrong, as they break symmetry with most of the current code style in the UI crate.

crates/matrix-sdk-ui/src/sync_service.rs

bnjbvr · 2025-01-16T16:40:01Z

crates/matrix-sdk-ui/src/sync_service.rs

+        match inner.state.get() {
            State::Idle | State::Terminated | State::Error => {
                // No need to stop if we were not running.
-                return Ok(());
+                Ok(())
            }
-            State::Running => {}
-        };
-
-        trace!("pausing sync service");
+            State::Running => {
+                trace!("pausing sync service");

-        // First, request to stop the two underlying syncs; we'll look at the results
-        // later, so that we're in a clean state independently of the request to
-        // stop.
+                // First, request to stop the two underlying syncs; we'll look at the results
+                // later, so that we're in a clean state independently of the request to stop.

-        // Remove the supervisor from our inner state and request the tasks to be
-        // shutdown.
-        let supervisor = inner.supervisor.take().ok_or_else(|| {
-            error!("The supervisor was not properly started up");
-            Error::InternalSupervisorError
-        })?;
+                // Remove the supervisor from our inner state and request the tasks to be
+                // shutdown.
+                let supervisor = inner.supervisor.take().ok_or_else(|| {
+                    error!("The supervisor was not properly started up");
+                    Error::InternalSupervisorError
+                })?;

-        supervisor.shutdown().await
+                supervisor.shutdown().await
+            }
+        }


This change feels like a regression: it adds one extra level of indent, for absolutely no extra value. I see the point of doing it in the other function because the body's size is so small there (in particular, there's no control flow), but it's not the case here, so this just adds visual clutter :/

The body size of the SyncService::start() and SyncService::stop() methods was reduced to just a couple of lines in a previous commit, allowing us to do exactly this without introducing visual clutter from indentation.

The value becomes clear when you consider what needs to happen if someone adds another enum variant—something I plan to do.

Consider the examples:

First, the pattern where we flatten out one branch to remove the indentation:

enum Foo { A, B, } fn bar(foo: Foo) { match foo { // Do nothing if Foo::A. A => return, B => (), } // Do what we would have done in the B branch. call_something_for_b(); }

Now the way it is in the pull-request:

enum Foo { A, B, } fn bar(foo: Foo) { match foo { // Do nothing if Foo::A. A => (), // Do what we need to do for B. B => { call_something_for_b(); }, } }

When we add a new variant, I’ll either need to convert the first variant into the second, or introduce even more early returns, which would make the flow even less clear:

enum Foo { A, B, C, } fn bar(foo: Foo) { match foo { // Do nothing if Foo::A. A => return, B => (), C => { // Do what we need for C, can't be flattened out like we did for B. do_something_for_c(); return; }, } // Do what we would have done in the B branch. call_something_for_b(); }

I hope we can agree that adding more variants would make the flow of execution even more convoluted.

In contrast, the second variant naturally provides a clear place to add a new branch, that's precisely what pattern matching is here for.

enum Foo { A, B, C, } fn bar(foo: Foo) { match foo { A => (), B => call_something_for_b(), C => do_something_for_c(), } }

I would urge you to reconsider your stance on this pattern and code style, and to evaluate how flattening the branches of a pattern match might affect the extensibility of the codebase.

Early returns have their, in my opinion, limited use—but this ain't it, babe. 🎶

That you planned to add another variant is something I wasn't aware of, and couldn't guess, so that makes sense in this case 👍

I would urge you to reconsider your stance on this pattern and code style, and to evaluate how flattening the branches of a pattern match might affect the extensibility of the codebase.

No, I'd rather have you reconsider your stance: more early returns make for fewer indent levels, and more "linear" / "simple" / "beautiful" code, where all the assumptions are checked upfront, and then the actual work is done later. Agree to disagree here :-)

That you planned to add another variant is something I wasn't aware of, and couldn't guess, so that makes sense in this case

You seem to be missing the point. Every enum might be expanded to include more variants, the point is to write code that is prepared to easily and exhaustively handle this scenario. The commit did mention this:

Now that the various match branches in the start and stop method of the SyncService are minimized we can remove the early returns. This should allow us to more easily add new branches.

No, I'd rather have you reconsider your stance

Well I did, otherwise I would not have rewritten this. I explained the problems such code cause multiple times in this very thread, so could you do your due diligence please as well?

We can talk about this privately.

⚠️ Actually, your argument doesn't hold. Were we to add a new variant to State, the compiler would've complained in the original form of the code that it's not handled in this match, giving us the pause that we're aiming for (to decide how to handle it). So changing the code like you did here is only a regression in terms of readability, and doesn't buy us any type checks that we didn't have before. Can you revert it, please?

That's still not the point I was making.

f522a81

crates/matrix-sdk-ui/src/sync_service.rs

codecov · 2025-01-16T18:31:40Z

Codecov Report

Attention: Patch coverage is 79.56989% with 19 lines in your changes missing coverage. Please review.

Project coverage is 85.43%. Comparing base (1d83d42) to head (f522a81).
Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/matrix-sdk-ui/src/sync_service.rs	79.56%	19 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4543      +/-   ##
==========================================
+ Coverage   85.42%   85.43%   +0.01%     
==========================================
  Files         285      285              
  Lines       32222    32229       +7     
==========================================
+ Hits        27525    27536      +11     
+ Misses       4697     4693       -4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

andybalaam

Looks great, thank you for doing this.

Previously we had a lock protecting an empty value, but the logic wants to protect a bunch of data in the SyncService. Let's do the usual thing and create a SyncServiceInner which holds the data and protect that with a lock.

From cambridge a scheduler is defined as: > someone whose job is to create or work with schedules While supervisor is defined as: > a person whose job is to supervise someone or something Well ok, that doesn't tell us much, supervise is defined as: > to watch a person or activity to make certain that everything is done correctly, safely, etc.: In conclusion, supervising a task is the more common and better understood terminology here I would say.

The supervisor is defined as two optional fields that are set and removed at the same time. This patch converts the two optional fields into a single optional struct. The fields inside the struct now aren't anymore optional. This ensures that they are always set and destroyed at the same time.

…action

Now that the various match branches in the start and stop method of the SyncService are minimized we can remove the early returns. This should allow us to more easily add new branches.

…pervisor

bnjbvr

I've looked at all the commits now. The commit message for the Scheduler -> Supervisor renaming was not really convincing, but the offline discussion where you referred to the computing-specific definition of supervisor had me agree it's a good name.

My two blocking comments are the ones starting with a ⚠️ emoji, otherwise mostly nits here and there. I'd like to take another look before approving. Thanks!

crates/matrix-sdk-ui/src/sync_service.rs

bnjbvr · 2025-01-23T10:53:25Z

crates/matrix-sdk-ui/src/sync_service.rs

+        match inner.state.get() {
            State::Idle | State::Terminated | State::Error => {
                // No need to stop if we were not running.
-                return Ok(());
+                Ok(())
            }
-            State::Running => {}
-        };
-
-        trace!("pausing sync service");
+            State::Running => {
+                trace!("pausing sync service");

-        // First, request to stop the two underlying syncs; we'll look at the results
-        // later, so that we're in a clean state independently of the request to
-        // stop.
+                // First, request to stop the two underlying syncs; we'll look at the results
+                // later, so that we're in a clean state independently of the request to stop.

-        // Remove the supervisor from our inner state and request the tasks to be
-        // shutdown.
-        let supervisor = inner.supervisor.take().ok_or_else(|| {
-            error!("The supervisor was not properly started up");
-            Error::InternalSupervisorError
-        })?;
+                // Remove the supervisor from our inner state and request the tasks to be
+                // shutdown.
+                let supervisor = inner.supervisor.take().ok_or_else(|| {
+                    error!("The supervisor was not properly started up");
+                    Error::InternalSupervisorError
+                })?;

-        supervisor.shutdown().await
+                supervisor.shutdown().await
+            }
+        }


⚠️ Actually, your argument doesn't hold. Were we to add a new variant to State, the compiler would've complained in the original form of the code that it's not handled in this match, giving us the pause that we're aiming for (to decide how to handle it). So changing the code like you did here is only a regression in terms of readability, and doesn't buy us any type checks that we didn't have before. Can you revert it, please?

crates/matrix-sdk-ui/src/sync_service.rs

crates/matrix-sdk-ui/Cargo.toml

crates/matrix-sdk-ui/src/sync_service.rs

Termination aligns better with the existing terminology.

… its own task

…thod

…branch

Hywan

I see many improvements in this pull request, thank you for taking time working on this!

5 locks replaced by a single one,
less uninitialized states by replacing 2 Option (on fields) by a single 1 (on a struct),
less clones,
a lot more documentation
and more details.

I've left a couple of comments that you may or may not to address; up to you.

Hywan · 2025-01-27T13:20:04Z

crates/matrix-sdk-ui/src/sync_service.rs

+            inner: Arc::new(AsyncMutex::new(SyncServiceInner {
+                scheduler_task: None,
+                scheduler_sender: None,
+                room_list_service,
+                encryption_sync_service: encryption_sync,
+                state,
+                encryption_sync_permit,
+            })),


Replacing 5 locks by 1 lock, that's what I call an improvement. Well done!

Hywan · 2025-01-27T13:21:30Z

crates/matrix-sdk-ui/src/sync_service.rs

@@ -72,18 +72,18 @@ struct SyncServiceInner {
    encryption_sync_service: Arc<EncryptionSyncService>,
    state: SharedObservable<State>,
    encryption_sync_permit: Arc<AsyncMutex<EncryptionSyncPermit>>,
-    /// Scheduler task ensuring proper termination.
+    /// Supervisor task ensuring proper termination.


I agree it's a better name according to your definitions. Scheduler wasn't “wrong” neither, but I believe Supervisor is more adapted.

Hywan · 2025-01-27T13:22:57Z

crates/matrix-sdk-ui/src/sync_service.rs

-    supervisor_task: Option<JoinHandle<()>>,
+/// A supervisor that starts two sync tasks, one for the room list and one for
+/// the end-to-end encryption support.
+struct SyncTaskSupervisor {
+    /// The task that supervises the two sync tasks.
+    task: JoinHandle<()>,
    /// `TerminationReport` sender for the [`Self::stop()`] function.
    ///
    /// This is set at the same time as all the tasks in [`Self::start()`].
-    supervisor_sender: Option<Sender<TerminationReport>>,


Replacing 2 Options by 1 reduces the states in this code, well done. Since it is for the struct itself, and not for its fields, the complexity is also reduced.

Hywan · 2025-01-27T13:24:08Z

crates/matrix-sdk-ui/src/sync_service.rs

-        receiver: Receiver<TerminationReport>,
-    ) -> Self {
+    async fn new(inner: &SyncServiceInner) -> Self {
+        let (sender, receiver) = tokio::sync::mpsc::channel(16);


Are we sure 16 is enough? Don't we want an unbounded channel, or a channel with a larger buffer?

crates/matrix-sdk-ui/src/sync_service.rs

Hywan · 2025-01-27T13:30:21Z

crates/matrix-sdk-ui/src/sync_service.rs

@@ -183,33 +183,38 @@ impl SyncTaskSupervisor {
        (room_list_task, encryption_sync_task)
    }

+    fn check_if_expired(err: &matrix_sdk::Error) -> bool {


That's an improvement. It can even be unit tested 😇.

Hywan · 2025-01-27T13:35:00Z

crates/matrix-sdk-ui/src/sync_service.rs

+/// A high level manager for your Matrix syncing needs.
+///
+/// The [`SyncService`] is responsible for managing real-time synchronization
+/// with a Matrix server. It can initiate and maintain the necessary
+/// synchronization tasks for you.


Thanks! That's exactly what we often miss to do in the SDK, this kind of doc is really useful.

Hywan · 2025-01-27T13:35:56Z

crates/matrix-sdk-ui/src/sync_service.rs

+            room_list_service,
+            encryption_sync_permit,


Getting better and better!

Hywan · 2025-01-27T13:36:36Z

crates/matrix-sdk-ui/src/sync_service.rs

@@ -73,7 +73,7 @@ struct SyncTaskSupervisor {
    task: JoinHandle<()>,
    /// [`TerminationReport`] sender for the [`SyncTaskSupervisor::shutdown()`]
    /// function.
-    abortion_sender: Sender<TerminationReport>,
+    termination_sender: Sender<TerminationReport>,


Agree on this.

Hywan · 2025-01-27T13:37:04Z

crates/matrix-sdk-ui/src/sync_service.rs

+/// A supervisor responsible for managing two sync tasks: one for handling the
+/// room list and another for supporting end-to-end encryption.
+///
+/// The two sync tasks are spawned as child tasks and are contained within t


- within t + within the

poljar requested a review from a team as a code owner January 16, 2025 16:27

poljar requested review from andybalaam and removed request for a team January 16, 2025 16:27

poljar force-pushed the poljar/offline-mode-sync-service-pr branch from 0bc9547 to 5f62220 Compare January 16, 2025 16:38

bnjbvr requested changes Jan 16, 2025

View reviewed changes

andybalaam approved these changes Jan 20, 2025

View reviewed changes

poljar added 10 commits January 22, 2025 16:09

refactor(ui): Move the spawning of the child tasks into the supervisor

7cf816d

refactor(ui): Move the expiration of the sync services closer to the …

fbf8139

…action

refactor(ui): Move the task spawning functions under the supervisor

18e7786

refactor(ui): Remove some early returns from the sync service

730b0a0

Now that the various match branches in the start and stop method of the SyncService are minimized we can remove the early returns. This should allow us to more easily add new branches.

refactor(ui): Prettify the two sync tasks a bit

2de262c

fix(ui): Shutdown the child tasks if the channel got closed in the su…

c08e186

…pervisor

docs(ui): Polish the documentation of the SyncService a bit

dbde96b

poljar force-pushed the poljar/offline-mode-sync-service-pr branch from b38e8b9 to dbde96b Compare January 22, 2025 15:17

poljar enabled auto-merge (rebase) January 22, 2025 15:18

docs: Document that the SyncService requires MSC4186

e151ea9

poljar force-pushed the poljar/offline-mode-sync-service-pr branch from d33383f to e151ea9 Compare January 23, 2025 09:50

bnjbvr requested changes Jan 23, 2025

View reviewed changes

poljar added 8 commits January 23, 2025 12:44

refactor(ui): Remove some unneeded references from the SyncServiceInner

d05784b

fixup! refactor(ui): Create a Supervisor for the SyncService

2062861

fixup! docs(ui): Polish the documentation of the SyncService a bit

ea3c044

refactor(ui): Rename the abortion sender in the SyncService

6e8438a

Termination aligns better with the existing terminology.

fixup! docs: Document that the SyncService requires MSC4186

07f86de

docs(ui): Clarify that the supervisor encapsulates the child tasks in…

7707997

… its own task

doc(ui): Reword the doc comment for the is_supervisor_task_running me…

5bd85fc

…thod

chore(ui): Move the SyncService stop logic out of the State::Running …

f522a81

…branch

Hywan approved these changes Jan 27, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor and clean up the SyncService #4543

Refactor and clean up the SyncService #4543

poljar commented Jan 16, 2025

bnjbvr left a comment

bnjbvr Jan 16, 2025

poljar Jan 17, 2025 •

edited

Loading

bnjbvr Jan 20, 2025

poljar Jan 22, 2025

bnjbvr Jan 22, 2025

bnjbvr Jan 23, 2025

poljar Jan 24, 2025

codecov bot commented Jan 16, 2025 •

edited

Loading

andybalaam left a comment

bnjbvr left a comment

bnjbvr Jan 23, 2025

Hywan left a comment •

edited

Loading

Hywan Jan 27, 2025

Hywan Jan 27, 2025

Hywan Jan 27, 2025

Hywan Jan 27, 2025

Hywan Jan 27, 2025

Hywan Jan 27, 2025

Hywan Jan 27, 2025

Hywan Jan 27, 2025

Hywan Jan 27, 2025

Refactor and clean up the SyncService #4543

Are you sure you want to change the base?

Refactor and clean up the SyncService #4543

Conversation

poljar commented Jan 16, 2025

bnjbvr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poljar Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 16, 2025 • edited Loading

Codecov Report

andybalaam left a comment

Choose a reason for hiding this comment

bnjbvr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hywan left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poljar Jan 17, 2025 •

edited

Loading

codecov bot commented Jan 16, 2025 •

edited

Loading

Hywan left a comment •

edited

Loading