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.

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

Rolling PR for the clients engine #1504

Merged
merged 13 commits into from
Sep 17, 2019
Merged

Rolling PR for the clients engine #1504

merged 13 commits into from
Sep 17, 2019

Conversation

linabutler
Copy link
Contributor

@linabutler linabutler commented Aug 1, 2019

The clients engine is used to send and receive commands for sync
engines, like wiping data and resetting Sync metadata. These are
sent in a client record's commands list, and acked by removing
the command from the list. The engine syncs twice: once at the
start of the sync, to apply any incoming commands from other clients,
and once at the end, to upload any outgoing commands added as a result
of the sync.

Our implementation is based on Desktop's, but works a little
differently:

  • Instead of holding commands in memory, ours pulls outgoing commands
    directly from logins and Places. This follows the approach of
    bookmark restore on Desktop, which writes a flag into Places to send
    a wipeEngine command on the next sync.
  • Our engine relies on the embedding app to supply it with a Settings
    struct containing info about the current client.

Thanks to these two changes, our engine doesn't need to store any state
itself. This means it can be used as a short-lived object that's dropped
after it syncs, like our other syncable stores, and not a long-lived
singleton as on Desktop.

Closes #1380.

Pull Request checklist

  • Quality: This PR builds and tests run cleanly
    • cargo test --all produces no test failures
    • cargo clippy --all --all-targets --all-features runs without emitting any warnings
    • cargo fmt does not produce any changes to the code
    • ./gradlew ktlint detekt runs without emitting any warnings
    • swiftformat --swiftversion 4 megazords components/*/ios && swiftlint runs without emitting any warnings or producing changes
    • Note: For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry in CHANGES_UNRELEASED.md or an explanation of why it does not need one
    • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
  • Dependencies: This PR follows our dependency management guidelines
    • Any new dependencies are accompanied by a summary of the due dilligence applied in selecting them.

@linabutler
Copy link
Contributor Author

@thomcc This is just a start (needs to be wired up to the sync manager, and tests!), but does the design seem sensible to you?

@thomcc thomcc force-pushed the smanager branch 5 times, most recently from 5533f29 to a23caeb Compare September 3, 2019 17:28
@linabutler linabutler force-pushed the clients-choo-choo branch 2 times, most recently from 503aa71 to dac0595 Compare September 3, 2019 19:19
@linabutler
Copy link
Contributor Author

OK, I think this is ready for review! 😅 The latest PR splits the implementation in two:

  • A clients engine, in the sync15 crate. It depends on a lot of sync15 internal structs, and, rather than expose them publicly, it made more sense to lift the engine into this crate. It exposes a CommandProcessor trait for handling the actual commands.
  • An implementation of a command processor in the sync manager, which does all the heavy lifting.

Also a few things to note:

  • We use the FxA device ID as the client record ID, per @mhammond's proposal.
  • A command can be applied, ignored, or unsupported (I wonder if that should be spelled Retry?). Unsupported commands—say, Wipe("passwords") for a client that only supports "bookmarks" and "history", or a totally new command that the client doesn't support at all—get put back in that client's record, on the chance that it's upgraded to support that data type at a later time.
  • Any errors syncing the clients engine abort the entire sync, including failing to run a command. This seems a bit excessive, but avoids weirdness like accidentally merging our local bookmarks with new ones on the server, instead of erasing all our bookmarks and starting over, if another device sent us a Wipe("bookmarks") command.

@linabutler linabutler marked this pull request as ready for review September 13, 2019 01:13
@linabutler linabutler requested a review from thomcc September 13, 2019 01:14
@linabutler
Copy link
Contributor Author

Also, this still needs tests! 😂

@rfk
Copy link
Contributor

rfk commented Sep 13, 2019

Unsupported commands—say, Wipe("passwords") for a client that only supports "bookmarks" and
"history", or a totally new command that the client doesn't support at all—get put back in that
client's record, on the chance that it's upgraded to support that data type at a later time.

Naively, this sounds kind of risky...are there any circumstances where delaying one of these commands and executing it much later could do weird things to the user's sync data?

@linabutler
Copy link
Contributor Author

Naively, this sounds kind of risky...are there any circumstances where delaying one of these commands and executing it much later could do weird things to the user's sync data?

Good question!

  • WipeAll and ResetAll only wipe and reset the engines we support at the time we process the command, so those won't be delayed.
  • Wipe("engine-that-we-dont-support-yet") and Reset("engine-that-we-dont-support-yet") are trickier. Let's say Fenix gets a Wipe("passwords") command, and puts it back in its record because it doesn't sync logins. Later, Fenix gets support for logins, and automatically imports them from Fennec before syncing. When the user upgrades, all their logins will get imported from Fennec, and then wiped when sync runs.

This could either be:

  • Desirable, if the user restored their logins on another device, and expects all devices to now reflect that view. If Fennec still had old logins, those will either be reuploaded or merged. (This is more of a concern with bookmark restores—we'll merge instead of wiping and starting over with the server's data!)
  • The worst thing to have happened! What if they accidentally erased all their passwords, and the Fennec import was the last copy they had?

For other commands...it depends on the command. But perhaps erring on the side of dropping the command would be the prudent choice.

(This also doesn't impact round-tripping commands that we don't understand for other clients...those will still get reuploaded).

@thomcc thomcc force-pushed the smanager branch 2 times, most recently from 34e25ce to ef62758 Compare September 16, 2019 22:13
Copy link
Contributor

@thomcc thomcc left a comment

Choose a reason for hiding this comment

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

This looks great with some nits addressed and rebased again.

components/sync_manager/src/manager.rs Show resolved Hide resolved
components/sync_manager/src/manager.rs Show resolved Hide resolved
components/sync15/src/clients/ser.rs Show resolved Hide resolved
components/sync15/src/sync_multiple.rs Outdated Show resolved Hide resolved
components/sync_manager/src/manager.rs Outdated Show resolved Hide resolved
@linabutler
Copy link
Contributor Author

(I just rebased, added history wiping, and some tests, but didn't address any of the other feedback yet. I'd also like to add a test for apply_incoming, which will involve rejiggering a few things so that we don't have to mock the whole world to make an Engine).

@linabutler linabutler changed the base branch from smanager to master September 17, 2019 20:34
The clients engine handles commands for sync engines, like wiping local
data and resetting Sync metadata. These are sent in a client record's
`commands` list, and acked by removing the command from the list.

Our implementation is based on Desktop's, but works a little
differently:

* Instead of holding commands in memory, ours pulls outgoing commands
  directly from logins and Places. This follows the approach of
  bookmark restore on Desktop, which writes a flag into Places to send
  a `wipeEngine` command on the next sync.
* Our engine relies on the embedding app to supply it with info about
  the current client.

Thanks to these two changes, our engine doesn't need to store any state
itself. This means it can be used as a short-lived object that's dropped
after it syncs, like our other syncable stores, and not a long-lived
singleton as on Desktop.
This calls the existing `history::delete_everything`. For symmetry,
this commit also renames `bookmarks::erase_everything` to match.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement the Clients engine and integrate it with the sync manager
3 participants