Sync on collection update

[sunkup]

Purpose

Nice to have for:

• When a collection gets enabled you would usually always want to sync it right away.
• Force-read-only setting: gets set/updated at sync only (see "Force read-only" setting not effective immediately #851).

Short description

• Pass account of the changed collection along when notifying listeners of collections change.
• Then have SyncWorkerManager listen to any collection changes and use the account passed along to
• then trigger a short delay-coroutine job, which resets itself on new change observations and after delay expiration
• enqueues a one time sync of the whole account (not just the service which the collection belongs to - although that would prob be possible as well).

Checklist

• The PR has a proper title, description and label.
• I have self-reviewed the PR.
• I have added documentation to complex functions and functions that can be used by other modules.
• I have added reasonable tests or consciously decided to not add tests.

[rfc2822]

I wonder how this behaves when the services are refreshed. Doesn't it enqueue a lot of syncs during service detection (whenever a collection is updated)?

My ideas (may or may not make sense):

• A delay of a few seconds between the collection change and the sync (like for the push registration) could at least reduce the number of synchronizations during service detection.
• Or maybe pause it somehow?
• Maybe separate callbacks for the actions we want to observe (update read-only, update sync flag)? Has advantages and disadvantages.

[rfc2822]

Hm, because we're not sure about unwanted consequences (regarding service detection etc)

Maybe we could the sync from the UI? So that not the DB Collection is observed, but instead when the user changes the read-only / sync flag in the UI, it waits a few seconds and then starts a sync?

[sunkup]

Hm, because we're not sure about unwanted consequences (regarding service detection etc)

Maybe we could the sync from the UI? So that not the DB Collection is observed, but instead when the user changes the read-only / sync flag in the UI, it waits a few seconds and then starts a sync?

Sure, we should drop #1230 then though, since we don't want that feature anymore?

[rfc2822]

Sure, we should drop #1230 then though, since we don't want that feature anymore?

I mean we could sync when they're (un)checked by the user in the UI, but not when the sync flag is modified by for instance some background code.

[rfc2822]

But maybe let's do it after the other 4.5 things and could also be good to discuss (dis)advantages in a talk

[ArnyminerZ]

Works perfectly

[rfc2822]

I still wonder why we really want to couple it on the data layer and not in the UI layer.

Advantage of data layer: independent from UI – when collections are for instance changed by managed settings or other circumstances, sync would still run again

Risk of data layer: we don't know when collections are really updated. For instance, we may at some time update the collection properties (calendar name, color, …) during the first PROPFIND of a sync, as we have thought about often. In that case a sync could automatically trigger another sync.

I think the basic question is: what's our goal?

• Shall every change in a collection entry, in the database/repository, regardless of where it comes from, automatically trigger a sync?
• Or shall only UI changes (enable/disable collection, select read-only) automatically trigger a sync to reflect these changes?

I'm not sure yet, but I think it would be more safe to trigger this on UI layer instead, at least for now. What are your thoughts?

[sunkup]

I think the basic question is: what's our goal?

• Shall every change in a collection entry, in the database/repository, regardless of where it comes from, automatically trigger a sync?

Besides having OnChangeListener we could also have a SyncOnChangeListener, whichSyncOnCollectionsChangeListener would subscribe to. We would only notify for a sync to start when needed. That is when collection sync and the force-read-only setting change (are enabled or disabled).

I think that's still clean architecture and it would solve the issue of unintentional syncs of other collection changes.

I'm not sure yet, but I think it would be more safe to trigger this on UI layer instead, at least for now. What are your thoughts?

I think both ways can be equally safe. Going the UI route is probably a bit less complex.

[rfc2822]

If it reacts on DB changes, why not also when the title (→ calendar name), color etc. changes (during a collection list refresh)? So OnChangeListener would cover more cases that also could be useful than a SyncOnChangeListener.

Two things:

1. Shall we adapt sync enqueueing so that it replaces the current sync instead of keeping it? For this use case:

• User checks collection A to be synced.
• User waits 7 seconds. Sync starts.
• While the sync runs, user checks collection B to be synced.
• User waits 7 seconds. Sync is still running (let's say it takes some time), and it's not replaced nor is a new sync enqueued.
• User waits until sync is complete.
• User expects calendar B to be there because sync has finished. However it's not there.

We could use ExistingWorkPolicy.REPLACE, but then service detection (assuming again that it takes some time) would start, cancel, restart syncs several times. Sounds like it could cause problems.

With ExistingWorkPolicy.APPEND, it could append some unnecessary syncs.

2. And/or shall we add some logic that service detection can postpone the logic while it runs? Then we would have to determine where such logic fits best.

[sunkup]

If it reacts on DB changes, why not also when the title (→ calendar name), color etc. changes (during a collection list refresh)? So OnChangeListener would cover more cases that also could be useful than a SyncOnChangeListener.

This would increase the likelihood of enqueuing new unneccessary syncs though, no? Like you mentioned:

Risk of data layer: we don't know when collections are really updated. For instance, we may at some time update the collection properties (calendar name, color, …) during the first PROPFIND of a sync, as we have thought about often. In that case a sync could automatically trigger another sync.

I do think however this would could be neglectible if we replace only enqueued workers. So not replace a running sync worker (ExistingWorkPolicy.REPLACE) and not stack a ton of uneccessary syncs (ExistingWorkPolicy.APPEND), but only replace/update the pending sync request directly after the currently running one.

I think this way we could reduce the amount of unecessary syncs considerably. Unfortunately it seems like workmanager does not have this feature inbuilt. So if we'd want only the latest request to run after the currently running one, that means to have at most two "active" workers:

• No workers: Enqueue normally
• 1 running: Add 2nd to stack
• 1 running + 1 queued: Skip! (max 2 reached)

We would need to do some querying ourselves before appending new requests, like so:

val activeCount = WorkManager.getInstance(context)
    .getWorkInfosForUniqueWork("onetime-sync ...")
    .get()
    .count { 
         it.state == WorkInfo.State.RUNNING || it.state == WorkInfo.State.ENQUEUED
    }

if (activeCount < 2)
    WorkManager.getInstance(context).enqueueUniqueWork(
        "onetime-sync ...",
        ExistingWorkPolicy.APPEND,
        workRequest
    )

Now for your two things:

1. Solved by appending at most one more worker
2. If I understand correctly, you would have RefreshCollectionsWorker be enqueued and run, but then have it not do its actual work (inside doWork) based on some logic/check? I can't see how this is beneficial 🤔

I'd love to have a call on this issue too 🔄

[sunkup]

Discussed in call.

Too risky. Hard to oversee potential problems. Infinite detection-sync loops etc.

We will create a new PR with UI triggers to test the water first.