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

Sync access token caching, API simplification #3579

Merged
merged 2 commits into from Jul 10, 2019

Conversation

Projects
None yet
3 participants
@grigoryk
Copy link
Contributor

commented Jun 27, 2019

I've started pulling on one little thread, and ended up with a few more changes than initially anticipated.

Raison d'être for this PR - introducing access token caching for Sync.

  • Some background on the issue: Rust FirefoxAccount object maintains an in-memory cache of access tokens, keyed by 'scope'. During every sync, we "rehydrate" an instance of FirefoxAccount, starting with a fresh cache. We then obtain an access token from it to sync; this performs a network request (since the internal cache is empty), which is quite costly at scale for our services. This creates a situation when we may overwhelm our own servers with a large enough, actively syncing user base.
  • This PR adds a caching layer for sync authInfo objects. Sync workers no longer interact with the account directly, and instead look into the cache to obtain authentication info necessary for syncing. No more "talk to the FxA server before every sync".
    Account manager is responsible for keeping the cache up-to-date, and resetting it when necessary. Cache is currently updated: on startup (but only if access token has expired), on authentication, and when we recover from auth problems.

And this is where the "thread pulling" begins! In order to "own" the access token for sync, account manager needs to be aware of the "sync scope".
Before, we just relied on the application to specify that scope. Instead, I've changed account manager's constructor to take a SyncConfig object which allows consuming application to configure how sync should behave (enabled at all?, periodic syncing enabled? how often to sync? which stores should be synced?).
Ownership of the "sync manager" moved down the stack, from the application layer into the account manager.

Application is now expected to interact with sync only via AccountManager's sync method, which exposes an internal SyncManager instance (if sync is enabled).

Above changes were a good reason to move support classes from feature-sync and into services-firefox-account. Note that since "sync" is part of our "storage" modules, this change doesn't mean that you need to take an extra native dependency on your classpath simply if you need to use FxA. Thanks to concept-sync, actual "Firefox Sync" machinery (within libplaces) is still fully decoupled from FxA. feature-sync has been removed entirely.

Since we're churning the public API anyway, I took the chance to introduce a few more simplifications at the API layer:

  • 'SyncManager' interface was removed, since we're not expecting to have multiple implementations of it
  • 'Config' was renamed to 'ServerConfig'
  • 'DeviceTuple' was renamed to 'DeviceConfig'
  • account manager grew a new public API, 'setSyncConfig', which allows application to re-configure how it wants sync to behave
  • 'AuthInfo' was renamed to 'SyncAuthInfo', and a bunch of cleanup happened in that area
  • 'AccountObservable'@'onError' method was removed. The only error that could have been passed into it (unable to restore account) wasn't actionable by the application anyway, and none of the integrations did anything with that call

Documentation of public APIs and classes was improved.

Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry or does not need one
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

@grigoryk grigoryk requested a review from mozilla-mobile/act as a code owner Jun 27, 2019

@grigoryk grigoryk force-pushed the grigoryk:tokenCache branch 6 times, most recently from 9925c19 to b7298fd Jun 29, 2019

@grigoryk

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2019

@csadilek I think I like the API as it stands now. I've simplified it a bit further since we last chatted about it.

@grigoryk grigoryk force-pushed the grigoryk:tokenCache branch from b7298fd to 458aa56 Jul 2, 2019

@st3fan st3fan referenced this pull request Jul 2, 2019

Closed

Update to A-C 0.56.6 #3858

@grigoryk grigoryk force-pushed the grigoryk:tokenCache branch 3 times, most recently from 8def8ed to f2629e4 Jul 2, 2019

@pocmo pocmo referenced this pull request Jul 3, 2019

Closed

Release 0.56.6 #3632

0 of 3 tasks complete

@csadilek csadilek self-assigned this Jul 3, 2019

@csadilek
Copy link
Contributor

left a comment

Only had a few nits. I think this is simpler now and the public API looks much better.

One concern is that this is a big change for the upcoming point release, but at the same time seems low risk and it's fixing a critical bug. Let's chat about this part more. There's also #3631 to consider...

Show resolved Hide resolved ...rage-sync/src/main/java/mozilla/components/browser/storage/sync/Types.kt Outdated

// We have type definitions at the concept level, and "external" types defined within Places.
// In practice these two types are largely the same, and this file is the conversion point.

/**
* Conversion from a generic AuthInfo type into a type 'places' lib uses at the interface boundary.
*/
internal fun AuthInfo.into(): SyncAuthInfo {
internal fun mozilla.components.concept.sync.SyncAuthInfo.into(): SyncAuthInfo {

This comment has been minimized.

Copy link
@csadilek

csadilek Jul 3, 2019

Contributor

👍 I like this change...made the code easier to read overall. Maybe we can find a different name later, because into is still a bit hard to grasp e.g. unwrap() or toNative()?

This comment has been minimized.

Copy link
@grigoryk

grigoryk Jul 3, 2019

Author Contributor

Sure, happy to explore other names. I've used into as a direct carry-over from Rust, actually.
https://doc.rust-lang.org/std/convert/trait.Into.html

This comment has been minimized.

Copy link
@pocmo

pocmo Jul 4, 2019

Contributor

In Kotlin those are usually to*() methods.

Show resolved Hide resolved .../firefox-accounts/src/main/java/mozilla/components/service/fxa/Config.kt Outdated
Show resolved Hide resolved .../firefox-accounts/src/main/java/mozilla/components/service/fxa/Config.kt Outdated
Show resolved Hide resolved ...counts/src/main/java/mozilla/components/service/fxa/SyncAuthInfoCache.kt Outdated
syncManager
ServerConfig.release(CLIENT_ID, REDIRECT_URL),
DeviceConfig("A-C Logins Sync Sample", DeviceType.MOBILE, setOf()),
SyncConfig(setOf("logins"))

This comment has been minimized.

Copy link
@csadilek

csadilek Jul 3, 2019

Contributor

👍 this looks much better now!

@grigoryk grigoryk changed the title WIP Sync access token caching, API simplification Sync access token caching, API simplification Jul 3, 2019

@grigoryk grigoryk force-pushed the grigoryk:tokenCache branch 3 times, most recently from 9e857ec to 8308693 Jul 8, 2019

Move sync scope ownership into account manager; API simplification
I've started pulling on one little thread, and ended up with a few more changes than initially anticipated.

Raison d'être for this PR - introducing access token caching for Sync.
- Some background on the issue: Rust FirefoxAccount object maintains an in-memory cache of access tokens, keyed by 'scope'. During every sync, we "rehydrate" an instance of FirefoxAccount, starting with a fresh cache. We then obtain an access token from it to sync; this performs a network request (since the internal cache is empty), which is quite costly at scale for our services. This creates a situation when we may overwhelm our own servers with a large enough, actively syncing user base.
- This PR adds a caching layer for sync authInfo objects. Sync workers no longer interact with the account directly, and instead look into the cache to obtain authentication info necessary for syncing. No more "talk to the FxA server before every sync".
Account manager is responsible for keeping the cache up-to-date, and resetting it when necessary. Cache is currently updated: on startup (but only if access token has expired), on authentication, and when we recover from auth problems.

And this is where the "thread pulling" begins! In order to "own" the access token for sync, account manager needs to be aware of the "sync scope".
Before, we just relied on the application to specify that scope. Instead, I've changed account manager's constructor to take a SyncConfig object which allows consuming application to configure how sync should behave (enabled at all?, periodic syncing enabled? how often to sync? which stores should be synced?).
Ownership of the "sync manager" moved down the stack, from the application layer into the account manager.

Application is now expected to interact with sync only via AccountManager's `sync` method, which exposes an internal SyncManager instance (if sync is enabled).

Above changes were a good reason to move support classes from feature-sync and into services-firefox-account. Note that since "sync" is part of our "storage" modules, this change doesn't mean that you need to take an extra native dependency on your classpath simply if you need to use FxA. Thanks to concept-sync, actual "Firefox Sync" machinery (within libplaces) is still fully decoupled from FxA. `feature-sync` has been removed entirely.

Since we're churning the public API anyway, I took the chance to introduce a few more simplifications at the API layer:
- 'SyncManager' interface was removed, since we're not expecting to have multiple implementations of it
- 'Config' was renamed to 'ServerConfig'
- 'DeviceTuple' was renamed to 'DeviceConfig'
- account manager grew a new public API, 'setSyncConfig', which allows application to re-configure how it wants sync to behave
- 'AuthInfo' was renamed to 'SyncAuthInfo', and a bunch of cleanup happened in that area
- 'AccountObservable'@'onError' method was removed. The only error that could have been passed into it (unable to restore account) wasn't actionable by the application anyway, and none of the integrations did anything with that call

Documentation of public APIs and classes was improved.

@grigoryk grigoryk force-pushed the grigoryk:tokenCache branch from b700662 to d34bbd5 Jul 10, 2019

@grigoryk grigoryk force-pushed the grigoryk:tokenCache branch from d34bbd5 to 589087f Jul 10, 2019

@grigoryk

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

This is ready to land. Waiting for green CI.

@grigoryk grigoryk merged commit 83befd9 into mozilla-mobile:master Jul 10, 2019

1 check passed

Taskcluster (pull_request) TaskGroup: success
Details

@grigoryk grigoryk deleted the grigoryk:tokenCache branch Jul 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.