Change Android Preference PlatformRemove and PlatformClear to be asynchronous

[csdinon]

Description

The implementation of Preferences for Android uses SharedPreferences.Editor.Commit for PlatformRemove and PlatformClear when editing. The docs only suggest using this if you're using the return value. I'm suggesting we use Apply to be consistent with PlatformSet and take unwanted disk writes off the main thread. https://developer.android.com/reference/android/content/SharedPreferences.Editor#commit()

Expected Behavior

Asynchronous disk writes when removing and clearing SharedPreferences.

Basic Information

https://developer.android.com/reference/android/content/SharedPreferences.Editor#commit()

VS bug #744070

[jamesmontemagno]

@Redth I am pretty sure we discussed this and went with Commit for a reason.

[Redth]

We discussed using Commit instead of Apply since commit is synchronous and it seemed like there were some reports of users getting stale values back from multiple shared preference instances and unexpected behaviour when they cleared preferences and then went to read them back out. There were a few SO posts we considered looking at this one and ultimately decided the small performance hit in some cases was favourable to the unexpected behaviour.

I'm curious what context you're using this in that the performance is causing issues?

[csdinon]

The only case documented is applications with multiple processes, but would Commit actually solve the issue?

So we work on various Xamarin apps. Clients assume preferences won't be doing disk io on the main thread so they set/remove preferences on the main thread quite frequently which leads to hiccups in UI updates.

small performance hit in some cases

is a bad characterization, it does io, depending on the system activity it can hang for an arbitrary amount of time. The filesystem will lock if the system or other apps are doing disk io.

[jamesmontemagno]

I think that in general Commit is the right direction as there is basically no overhead to dealing with sharedpreferences at all as I have been using it this way for 7 yeas ;). I think that how developers use Tasks and possibly background services and threads and since we use this API internally it is best to ensure that when the APIs are called they are actually called.

Any by small hit it really is under the threashold of implementing an async all in C#. As it is not measurable and would not cause UI update hiccups.

I can still be convinced if you are actually seeing something in your apps and have a reproduction.

[rf43]

as there is basically no overhead to dealing with sharedpreferences at all as I have been using it this way for 7 yeas ;).

@jamesmontemagno I am confused by your comment. Using commit() with SharedPreferences is a blocking I/O disk write. It is unknown at the time of calling it how long it will take to actually write to disk. This is why one would prefer .apply() over commit. The Android documentation even explicitly calls this out as a potential issue.

[csdinon]

I've been working with Android for 7 years as well ;)

there is basically no overhead to dealing with sharedpreferences

I'm unclear on what you define as "overhead" here. The Xamarin profiler shows 100ms-200ms on commits, which led me to look at the source.

From the docs:

If another editor on this SharedPreferences does a regular commit() while a apply() is still outstanding, the commit() will block until all async commits are completed as well as the commit itself.

This means if we set a bunch of preferences and then remove one, that remove will block on all the outstanding sets. It's incorrect to assume apps won't do this.

Also, the implementation is inconsistent. When PlatformSet is passed null we remove that asynchronously. Which doesn't match PlatformRemove which uses Commit.

https://github.com/xamarin/Essentials/blob/cb06948830f6ece7690f0a264b07196f88b8a5bf/Xamarin.Essentials/Preferences/Preferences.android.cs#L57

[jamesmontemagno]

Hmmm it seems odd that Google would allow it to consume that much time, but very interesting. Good point on the inconsistency... that seems odd... We should evaluate that. I would like to see how we can add a unit test for this to ensure that adding/removing/clearing all work as expected..... not sure what that one would look like..... thoughts on that?

[xamarin-release-manager]

[VS sync] The field 'Milestone' contains the value '1.1.0' that is not in the list of supported values