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
48 changes: 24 additions & 24 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 @@ -348,15 +348,16 @@ impl SyncService {
let mut inner = self.inner.lock().await;

// Only (re)start the tasks if any was stopped.
if matches!(self.state.get(), State::Running) {
// It was already true, so we can skip the restart.
return;
}

trace!("starting sync service");
inner.supervisor = Some(SyncTaskSupervisor::new(&inner).await);
match inner.state.get() {
// If we're already running, there's nothing to do.
State::Running => (),
State::Idle | State::Terminated | State::Error => {
trace!("starting sync service");

self.state.set(State::Running);
inner.supervisor = Some(SyncTaskSupervisor::new(&inner).await);
inner.state.set(State::Running);
}
}
}

/// Stop the underlying sliding syncs.
Expand All @@ -368,28 +369,27 @@ impl SyncService {
pub async fn stop(&self) -> Result<(), Error> {
let mut inner = self.inner.lock().await;

match self.state.get() {
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
}
}
Copy link
Member

Choose a reason for hiding this comment

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

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 :/

Copy link
Contributor Author

@poljar poljar Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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. 🎶

Copy link
Member

Choose a reason for hiding this comment

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

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 :-)

}

/// Attempt to get a permit to use an `EncryptionSyncService` at a given
Expand Down