-
Notifications
You must be signed in to change notification settings - Fork 20
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
KVault.set() uses sharedPreferences.commit() instead of sharedPreferences.apply() #49
Comments
@rishabh876 feel free to give it a try! |
@rishabh876 you can actually perform the KVault calls async, after some thinking I believe this is suboptimal, we do not really have an async alternative on iOS and just using commit does not guarantee that the shared prefs were written. Which seems to be okay (in some circumstances) for shared prefs but for secrets I would certainly want some "guarantee" rather than a "will probably be saved". You can reproduce cases in which data gets lost (commit and quick app crash or closing). |
Under the hood, val prefScope = CoroutineScope(Dispatchers.IO + prefMotherJob)
...
prefScope.launch {
KVault.set()...
} |
I tried moving set operation of KVault to IO thread. Turns out keystore are not thread safe on some devices. I started seeing
After digging a bit more I stumbled upon this thread. FlowCrypt/flowcrypt-android#235 (comment) which suggests its a Thread safety issue. I would highly recommend adding this thread-safety disclaimer in documentation of Kvault as well. It must always be used on main thread. |
Can someone make a release with this fix please? |
SharedPreference provides two ways to write changes i.e.,
commit()
andapply()
. Wherecommit()
is a synchronous call and can block Mainthread. Whereasapply()
write in-memory first followed by asynchronously to the disk.More details on apply() can be found here.
Can we add option to use apply() by introducing a new API
KVault.setAsync(... , ...)
?The text was updated successfully, but these errors were encountered: