f2prateek / rx-preferences

Reactive SharedPreferences for Android
http://f2prateek.com/2015/10/05/rx-preferences/
Apache License 2.0
1.54k stars 132 forks source link

apply() is lossy #119

Open kurtisnelson opened 6 years ago

kurtisnelson commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

sirknigget commented 4 years ago

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:

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()...