feat: add "on backgrounded" function #611

EvanHahn · 2024-05-06T21:58:54Z

MapeoManager#onBackgrounded() should be called when the app goes into the background. It will do its best to gracefully shut down sync.

Closes #576.

`MapeoManager#onBackgrounded()` should be called when the app goes into the background. It will do its best to gracefully shut down sync. Closes [#576]. [#576]: #576

src/sync/peer-sync-controller.js

src/sync/autostopper.js

src/sync/sync-api.js

gmaclennan

I find it quite hard to follow the logic here, and the overall approach doesn't "feel" quite right (sorry to be vague, maybe talking it through will help me get clear on specifics). I think what autostop is doing is stopping sync after a fixed period of time, rather than unreplicating cores when sync has finished, but I'm not sure because I don't completely understand the logic. I think it would be worth co-working on this a bit more.

src/sync/peer-sync-controller.js

Co-Authored-By: Gregor MacLennan <[email protected]>

EvanHahn · 2024-05-16T20:32:46Z

Simplified this a bunch (I think).

When the app is in the background and sync is complete, cores will be un-replicated. There is no longer an auto-stop timeout, but it should be easy to add in the future.

There are now two booleans: wantsToSyncData and isBackgrounded.

start() enables wantsToSyncData.
stop() disables wantsToSyncData.
background() enables isBackgrounded.
foreground() disables isBackgrounded.

After these operations and when the state is updated, #updateState() is called. This figures out the desired SyncEnabledState and applies it.

gmaclennan

Good work on this, the logic looks correct to me, but there is one place in particular where it seems very fragile around management of the syncEnabledState - it should work as-is, but only because of nuances of Map iteration order and how we read it. I think we should fix this before merging.

Summary of other comments:

We need to define the state.initial.syncing now, because currently it defaults to true always.
There are a few naming things that might help future maintainers understand things.
Finally I think it's not great to have duplicate logic for deriving syncEnabledState in both the #getState and #updateState methods.

src/sync/peer-sync-controller.js

gmaclennan · 2024-05-17T09:12:16Z

src/sync/sync-api.js

+    state.data.syncing =
+      this.#wantsToSyncData &&
+      (!this.#isBackgrounded ||
+        !isSynced(namespaceSyncState, 'full', this.#peerSyncControllers))
+


It probably makes sense to rename state['data' | 'initial'].syncing to state['data' | 'initial'].syncEnabled to be clear about what this means.

This is essentially calculating the value of syncEnabledState based on the same logic that is in #updateState(), and we need to remember to keep those two pieces of logic in sync. Hoisting syncEnabledState to somewhere that can be read here would avoid having this duplication.

For completeness we should also set state.initial.syncing here, because currently it just defaults to always be true, but now it can be false.

Refactored in 8fe34aa.

gmaclennan · 2024-05-17T09:18:54Z

src/sync/sync-api.js

@@ -50,7 +56,8 @@ export class SyncApi extends TypedEmitter {
  #pscByPeerId = new Map()
  /** @type {Set<string>} */
  #peerIds = new Set()
-  #isSyncing = false
+  #wantsToSyncData = false
+  #isBackgrounded = false


I think this naming (isBackgrounded) is confusing, because it doesn't describe what this does, it describes a state that only really makes sense on mobile, and it's behaviour is determined by platform specifics of how we want sync to behave when in the background. Understanding what it does requires reading the code, or understanding that we want sync to gracefully stop when complete when the app is in the background. I'm thinking ahead to when we integrate this into Desktop and Mapeo Cloud server. This is why in my hacking around yesterday I was moving towards the concept of starting and stopping both sync types (initial & data), and being more explicit about auto-starting initial sync on initialization (e.g. starting initial sync in the constructor).

I'm not great at names, but perhaps we can call this either wantsToStopAllSync or the inverse wantsToSyncInitial or wantsToSyncPresync. Then perhaps the methods can be kStopAllSyncGracefully and kResumeSync?

Renamed in 4a641e7.

gmaclennan · 2024-05-17T09:35:28Z

src/sync/sync-api.js

+      for (const peerSyncController of this.#peerSyncControllers.values()) {
+        isStopped = peerSyncController.syncEnabledState === 'none'
+        break
+      }


I think it starts to get quite confusing storing this state on the peer sync controllers, since a future maintainer could without understanding set a peer controller sync enabled state somewhere, so that this state is different for some PSCs, which would break this. There is also kind of vague behaviour around when PSC instances are created with syncEnabledState === 'none', before it is set from this class, because this class reads that state as the source of truth while also setting that state. It's kind of circular...

I suggest that the syncEnabled state is hoisted, either to this class or to the SyncState class, which is already passed to the PSC instances, so they could read syncState.syncEnabledState from there. That would also avoid the not-quite-determined behaviour when PSC instances are created, because they will read the shared value from SyncState, rather than having their own state until it starts being controlled by this class.

What I'm wanting to avoid here is implicit context knowledge in the codebase, e.g. "you must set the syncEnabledState of all PSCs at the same time, but you can't entirely rely on any one PSC's syncEnabledState to be accurate, so we should re-set syncEnabledState each time we update".

Fixed in ba539cf. (I should've left a PR comment saying that I didn't like the way I addressed this.)

Co-authored-by: Gregor MacLennan <[email protected]>

_I recommend reviewing this with whitespace changes disabled._ This test-only change drops Brittle from our FastifyController tests and replaces them with `node:test` and `node:assert`.

This test-only change drops Brittle from our basic manager tests and replaces them with `node:test` and `node:assert`. Two notable things about this change: 1. There was a test that was commented out because of a Brittle bug. This uncomments it and sets the `skip: true` option. 2. Brittle has an ([undocumented][0]) `t.snapshot()` method which we can't use. I replaced it with a custom snapshot assertion. The rest was just mechanical changes. [0]: holepunchto/brittle#45

This test-only change drops Brittle from our Fastify server tests and replaces them with `node:test` and `node:assert`.

This test-only change drops Brittle from our project settings tests and replaces them with `node:test` and `node:assert`.

Co-authored-by: Evan Hahn <[email protected]>

We plan to replace Brittle with `node:test` which doesn't have an equivalent for `t.plan`. Remove it to make it easier to migrate.

When you leave a project, we now delete all indexed data outside of the `auth` namespace. Fixes [#427]. [#427]: #427 Co-Authored-By: Andrew Chou <[email protected]>

EvanHahn · 2024-05-20T22:23:25Z

Thanks for all the review here. Hopefully this is the last round!

gmaclennan · 2024-05-20T22:59:51Z

src/sync/sync-api.js

    return state
  }

+  #updateState = () => {


Maybe add namespace sync state as an optional parameter, to save recalculating it on state events. It's expensive to calculate.

Done in db4ff6c.

transform.ts

feat: add "on backgrounded" function

966a0db

`MapeoManager#onBackgrounded()` should be called when the app goes into the background. It will do its best to gracefully shut down sync. Closes [#576]. [#576]: #576

EvanHahn requested a review from gmaclennan May 6, 2024 21:58

Merge branch 'main' into evanhahn/576

2cb7a0b

gmaclennan reviewed May 8, 2024

View reviewed changes

src/sync/peer-sync-controller.js Show resolved Hide resolved

Merge branch 'main' into evanhahn/576

0dd8afd

EvanHahn marked this pull request as draft May 9, 2024 14:15

EvanHahn added 4 commits May 9, 2024 14:45

Merge branch 'main' into evanhahn/576

e2fb950

Merge branch 'main' into evanhahn/576

0666dc7

Add auto-stop functionality

9b7470d

Simplify autostop

610d359

EvanHahn commented May 15, 2024

View reviewed changes

src/sync/peer-sync-controller.js Show resolved Hide resolved

src/sync/autostopper.js Outdated Show resolved Hide resolved

src/sync/sync-api.js Outdated Show resolved Hide resolved

Merge branch 'main' into evanhahn/576

93b697c

EvanHahn marked this pull request as ready for review May 15, 2024 18:49

EvanHahn requested a review from gmaclennan May 15, 2024 19:07

EvanHahn mentioned this pull request May 15, 2024

Add auto-stop option to $sync.start() #627

Closed

gmaclennan reviewed May 16, 2024

View reviewed changes

src/sync/peer-sync-controller.js Outdated Show resolved Hide resolved

src/sync/peer-sync-controller.js Show resolved Hide resolved

EvanHahn and others added 3 commits May 16, 2024 18:58

Rename "namespace group" to "sync enabled state"

b8c5d7d

Co-Authored-By: Gregor MacLennan <[email protected]>

Rename "initial" to "presync"

f427f64

Simplify background/foreground API with two booleans

072a2b6

EvanHahn force-pushed the evanhahn/576 branch from 5fb358a to 072a2b6 Compare May 16, 2024 20:28

Merge branch 'main' into evanhahn/576

2e5fcb7

EvanHahn requested a review from gmaclennan May 16, 2024 20:32

gmaclennan requested changes May 17, 2024

View reviewed changes

EvanHahn and others added 6 commits May 20, 2024 16:07

Minor optimization when changing syncEnabledState

8a9eb15

Co-authored-by: Gregor MacLennan <[email protected]>

test: move FastifyController tests to node:test (#643)

c629505

_I recommend reviewing this with whitespace changes disabled._ This test-only change drops Brittle from our FastifyController tests and replaces them with `node:test` and `node:assert`.

test: move Fastify server tests to node:test (#661)

2058116

This test-only change drops Brittle from our Fastify server tests and replaces them with `node:test` and `node:assert`.

test: move project settings tests to node:test (#660)

6fdc9f3

This test-only change drops Brittle from our project settings tests and replaces them with `node:test` and `node:assert`.

chore: add Node 20 to CI (#652)

2b6eb06

Co-authored-by: Evan Hahn <[email protected]>

EvanHahn and others added 6 commits May 20, 2024 21:07

test: remove t.plan() calls (#658)

660027a

We plan to replace Brittle with `node:test` which doesn't have an equivalent for `t.plan`. Remove it to make it easier to migrate.

feat: delete lots of data from indexers when leaving project (#469)

2126de6

When you leave a project, we now delete all indexed data outside of the `auth` namespace. Fixes [#427]. [#427]: #427 Co-Authored-By: Andrew Chou <[email protected]>

Move "previous sync enabled state" to SyncApi

ba539cf

Rename "is backgrounded" to "has requested full stop"

4a641e7

Rename syncing to isSyncEnabled

8fe34aa

Merge branch 'main' into evanhahn/576

dc9e5c1

EvanHahn requested a review from gmaclennan May 20, 2024 22:23

gmaclennan reviewed May 20, 2024

View reviewed changes

Add default param for #updateState

db4ff6c

gmaclennan reviewed May 21, 2024

View reviewed changes

transform.ts Outdated Show resolved Hide resolved

EvanHahn added 4 commits May 21, 2024 14:29

Merge branch 'main' into evanhahn/576

a71a149

Remove accidentally-committed jscodeshift file

f649a2b

Merge branch 'main' into evanhahn/576

c437176

Merge branch 'main' into evanhahn/576

9f43b1c

EvanHahn merged commit bb9b16c into main May 27, 2024
7 checks passed

EvanHahn deleted the evanhahn/576 branch May 27, 2024 22:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add "on backgrounded" function #611

feat: add "on backgrounded" function #611

EvanHahn commented May 6, 2024

gmaclennan left a comment

EvanHahn commented May 16, 2024

gmaclennan left a comment •

edited

Loading

gmaclennan May 17, 2024

gmaclennan May 17, 2024

EvanHahn May 20, 2024

gmaclennan May 17, 2024

EvanHahn May 20, 2024

gmaclennan May 17, 2024

gmaclennan May 17, 2024

EvanHahn May 20, 2024

EvanHahn commented May 20, 2024

gmaclennan May 20, 2024

EvanHahn May 20, 2024

feat: add "on backgrounded" function #611

feat: add "on backgrounded" function #611

Conversation

EvanHahn commented May 6, 2024

gmaclennan left a comment

Choose a reason for hiding this comment

EvanHahn commented May 16, 2024

gmaclennan 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

EvanHahn commented May 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmaclennan left a comment •

edited

Loading