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

KVault.set() uses sharedPreferences.commit() instead of sharedPreferences.apply() #49

Open
rishabh876 opened this issue Aug 25, 2023 · 5 comments

Comments

@rishabh876
Copy link

rishabh876 commented Aug 25, 2023

SharedPreference provides two ways to write changes i.e., commit() and apply(). Where commit() is a synchronous call and can block Mainthread. Whereas apply() 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(... , ...)?

@rishabh876 rishabh876 changed the title KVault.set() uses sharedPreference.commit() instead of sharedPreference.apply() KVault.set() uses sharedPreferences.commit() instead of sharedPreferences.apply() Aug 25, 2023
@benjohnde
Copy link
Member

@rishabh876 feel free to give it a try!

@benjohnde
Copy link
Member

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

@yuroyami
Copy link

Under the hood, .commit() simply executes the operation on the current thread, whilst .apply() delegates it to an ephemeral background thread not controlled by you. .commit() rather gives you thread control, you can make use of the kotlin coroutines to do what you're aiming for:

val prefScope = CoroutineScope(Dispatchers.IO + prefMotherJob)

...

prefScope.launch {
    KVault.set()...
}

@rishabh876
Copy link
Author

rishabh876 commented Jan 10, 2024

I tried moving set operation of KVault to IO thread. Turns out keystore are not thread safe on some devices. I started seeing

KeyStoreException the master key android-keystore://_androidx_security_master_key_ exists but is unusable

caused by -> IllegalBlockSizeException caused by -> KeyStoreException - Invalid operation handle

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.

@cristhianescobar
Copy link

Can someone make a release with this fix please?

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

4 participants