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

apply() is lossy #119

Open
kurtisnelson opened this issue Jul 25, 2018 · 7 comments
Open

apply() is lossy #119

kurtisnelson opened this issue Jul 25, 2018 · 7 comments

Comments

@kurtisnelson
Copy link

Using apply() to save preferences is dangerous as it can and will silently fail.
Instead, all sets should use commit() and return a completable, throwing an error if commit() returns false.

@JakeWharton
Copy link
Contributor

Somewhere you dropped an "optionally" from your feature request, right? Almost no one will want to use this as it destroys the ergonomics of the API and few will be using preferences for anything where the absence of durability is a real problem.

Prior art: #110 #42

@kurtisnelson
Copy link
Author

The existing API could at least track the failure somewhere.
At scale I've gotten many a bug report where a feature seemed to be not working where in fact a preference was not persisting. It's super handy to at least have the exception in my crash reporting system so I know what happened.

@sevar83
Copy link

sevar83 commented Aug 20, 2018

I think I've got a race condition because of the apply(). My theory is that when another observer subscribes before the data is actually updated within apply() then the observer will get stale data.

p.s. More about the problem: https://stackoverflow.com/questions/49593476/android-sharedpreferences-apply-race-condition Look at the Mark Keen's comment.

@kirillzh
Copy link
Contributor

I'm seeing that the library creates a new Preferences.Editor every time changes are made. Considering that apply() is synchronous and atomic but it's called from different Editor instances, would using the same instance of Editor fix the problem?

@sirknigget
Copy link

We also encountered a major problem because of apply(). We NEED the atomic persistence to disk.

I agree with not destroying the API, but why not expose the possibility from the library? Currently it's not possible to perform commit() using RxSharedPreferences.
How about some atomicSet() method?

@kurtisnelson
Copy link
Author

I ended up writing a library that provides a proper async key-value store: https://github.com/uber/simple-store

@sirknigget
Copy link

Still missing this basic functionality.
The current library completely hides basic Android API stuff that I would normally use, and I end up having to patch around RxSharedPreferences in order to be able to:

  • Call Editor.commit() when setting a preference.
  • Access the SharedPreferences field inside a RealPreference
    ( current it's:
    private final SharedPreferences preferences;
    )

In the current situation I have to either fork rx-preferences and change those library classes, patch around those limitations, or move to using another library.

I think it's a very small, easy to implement request, and it doesn't break anything.
Just add "setSync" to RealPreference which would call Editor.commit()...

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

No branches or pull requests

5 participants