Skip to content

[shared_preferences] Fix initialization race #4159

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

Merged
merged 2 commits into from
Jun 8, 2023

Conversation

stuartmorgan-g
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g commented Jun 7, 2023

During the NNBD transition, the structure of the completer logic in the initialization flow was incorrectly changed to set the field after an await instead of immediately.

Also updates the error handling to handle Error the same way it currently handles Exception, which this change surfaced.

Fixes flutter/flutter#42407

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

During the NNBD transition, the structure of the completer logic in the
initialization flow was incorrectly changed to set the field after an
`await` instead of immediately.

Fixes flutter/flutter#42407
@tarrinneal
Copy link
Contributor

Interesting. The setPrefix method is getting an unimplemented error on some tests now.

@stuartmorgan-g
Copy link
Contributor Author

Interesting. The setPrefix method is getting an unimplemented error on some tests now.

I missed that only half of the tests were in the group that sets an instance during setUp (making my change to instance affect other tests). Fixing now.

@stuartmorgan-g
Copy link
Contributor Author

GitHub isn't showing my push here yet since it's having some kind of semi-outage at the moment, but it's up at main...stuartmorgan:packages:shared-prefs-init

The test change looks enormous, but I just removed the generic group that most-but-not-all of the tests were in, so now they are all getting the same setup. (I'm not sure why the pattern in so many of our plugins is that there's a group containing all of the tests.)

Copy link
Contributor

@tarrinneal tarrinneal left a comment

Choose a reason for hiding this comment

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

I'm surprised this worked at all before.

@stuartmorgan-g
Copy link
Contributor Author

I'm surprised this worked at all before.

Well, to hit this race you have to call getInstance again between the very first time in the app that you call getInstance and the initial _getSharedPreferencesMap call completing, which is likely to be a very small window of time. And then for it to be an actual problem, you have to either keep both of those instances around long term, or you have to keep (the wrong) one of them long term, but then also sometimes make new getInstance calls.

I can't image that's a common use pattern.

@stuartmorgan-g stuartmorgan-g added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 8, 2023
@auto-submit auto-submit bot merged commit d935cb0 into flutter:main Jun 8, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 8, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jun 8, 2023
flutter/packages@a84b2c2...e13b8c4

2023-06-08 [email protected] [tool] Only run unit tests in Chrome for inline web (flutter/packages#4153)
2023-06-08 [email protected] [in_app_purchase] Make the _FeatureCard constructor const in the Android example app (flutter/packages#4162)
2023-06-08 [email protected] [shared_preferences] Fix initialization race (flutter/packages#4159)
2023-06-07 [email protected] [go_router] Refactors imperative APIs and browser history (flutter/packages#4134)
2023-06-07 [email protected] [various] Add `http` 1.0 compatibility (flutter/packages#4147)
2023-06-07 [email protected] [go_router_builder] Accept required parameters not in path (flutter/packages#4039)
2023-06-07 [email protected] Roll Flutter from 0b74153 to 8a5c22e (46 revisions) (flutter/packages#4160)
2023-06-07 [email protected] [pigeon] Require analyzer 5.13.0, prepare for NamedType refactoring. (flutter/packages#4127)
2023-06-07 [email protected] Roll Flutter (stable) from f92f441 to 682aa38 (1 revision) (flutter/packages#4157)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: shared_preferences
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SharedPreferences should have better singleton instantiation
2 participants