Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

Re-enable Nimbus with Off the main thread I/O and non-blocking networking #17684

Merged
merged 4 commits into from
Feb 1, 2021

Conversation

jhugman
Copy link
Contributor

@jhugman jhugman commented Jan 28, 2021

Fixes SDK-152.

In an effort to fix SDK-157 this PR brings together:

  1. SDK-170 Add a Threadsafe annotation for interfaces mozilla/uniffi-rs#372 — allow Rust objects to be marked as Threadsafe, to indicate to uniffi not to lock access to one method at a time.
  2. Make NimbusClient Send+Sync via mutexes and cells mozilla/nimbus-sdk#86 — refactor Nimbus to use its own locking, instead of relying on uniffi. This also introduces an in-memory cache of enrollments.
  3. Split update_experiments into two phases mozilla/nimbus-sdk#78 — split the long running, blocking and single threaded updateExperiments into a slow network fetch and a fast apply phase to re-calculate enrollments which writes to the database.
  4. Add methods to cover new methods in NimbusClient android-components#9394 — Move fetch calls and calls that mutate the internal state of Nimbus to two separate coroutine scopes.

Furthermore, this PR also: moves setup of the Nimbus object into its own file; starts using the new fetch and apply methods, and improves error resilience.

Once all of this lands, this fixes #17086. Indirectly, it fixes #17143.

Pull Request checklist

  • Tests: This PR includes thorough tests or an explanation of why it does not
  • [ ] Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not Has no UI
  • [ ] Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product. Has no UI.

To download an APK when reviewing a PR:

  1. click on Show All Checks,
  2. click Details next to "Taskcluster (pull_request)" after it appears and then finishes with a green checkmark,
  3. click on the "Fenix - assemble" task, then click "Run Artifacts".
  4. the APK links should be on the left side of the screen, named for each CPU architecture

@jhugman jhugman requested review from a team as code owners January 28, 2021 22:33
@jhugman jhugman force-pushed the jhugman/sdk-152-move-nimbus-init-otmt branch from a11fa07 to 9f6234f Compare January 28, 2021 22:45
@jhugman jhugman requested a review from a team as a code owner January 28, 2021 22:45
@jhugman
Copy link
Contributor Author

jhugman commented Jan 28, 2021

IMPORTANT: Only turn this back on once the following issues are resolved:

#17143 is fixed by way of SDK-161 and mozilla/nimbus-sdk#77 and extra try/catch protection around Nimbus setup.

@jhugman jhugman requested a review from csadilek January 28, 2021 22:54
@jhugman jhugman added eng:perf-impact This issue may have an impact on performance: the perf team will investigate pr:needs-ac-bump PR that needs a AC bump labels Jan 28, 2021
Copy link
Member

@travis79 travis79 left a comment

Choose a reason for hiding this comment

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

🎉

@@ -8,17 +8,14 @@ import android.app.Application
import android.app.PendingIntent
import android.content.Context
import android.content.Intent
import android.net.Uri
import android.os.StrictMode

Copy link
Member

Choose a reason for hiding this comment

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

nit: blank line

@jhugman jhugman force-pushed the jhugman/sdk-152-move-nimbus-init-otmt branch from 6302ee3 to d36e60e Compare January 29, 2021 20:55
@mergify
Copy link
Contributor

mergify bot commented Jan 31, 2021

This pull request has conflicts when rebasing. Could you fix it @jhugman? 🙏

@jhugman jhugman force-pushed the jhugman/sdk-152-move-nimbus-init-otmt branch from d36e60e to 5b028cd Compare February 1, 2021 14:56
Copy link
Contributor

@csadilek csadilek left a comment

Choose a reason for hiding this comment

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

We did a few rounds of manual testing, simulating slow networks and no network connection at all. Everything seems fine now :).

At this point we can land and should monitor Sentry and perf. results.

@jhugman jhugman force-pushed the jhugman/sdk-152-move-nimbus-init-otmt branch from 5b028cd to 3b4f83c Compare February 1, 2021 22:49
@jhugman jhugman merged commit 72f9777 into master Feb 1, 2021
@bors bors bot deleted the jhugman/sdk-152-move-nimbus-init-otmt branch February 1, 2021 23:50
@jhugman jhugman restored the jhugman/sdk-152-move-nimbus-init-otmt branch February 1, 2021 23:50
@jhugman jhugman deleted the jhugman/sdk-152-move-nimbus-init-otmt branch February 1, 2021 23:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
eng:perf-impact This issue may have an impact on performance: the perf team will investigate pr:needs-ac-bump PR that needs a AC bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Crashes initializing Nimbus [Bug] Large regression in MAIN/VIEW start up due to Nimbus network IO
3 participants