[shared_preferences] Fix concurrent modification of the shared preferences on Android

[konstantintuev]

When value is set before the last value has been committed to the shared preferences, the commit of the last value is rejected with the following error:
java.util.concurrent.RejectedExecutionException: Task io.flutter.plugins.sharedpreferences.MethodCallHandlerImpl rejected from java.util.concurrent.ThreadPoolExecutor.

This happens because a SynchronousQueue is being used, which can handle only one request at a time and isn't really queue unless a request is removed or done.
By replacing it with a LinkedBlockingQueue the commits are queue to execute one after another. This preserves the async nature of the library, prevents data loss and fixes the RejectedExecutionException.

This fixes this issue: [shared_preferences] 2.0.3 throws a lot of Java Tracebacks on Android

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

• The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
• I listed at least one issue that this PR fixes in the description above.
• I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt.
• I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
• I updated CHANGELOG.md to add a description of the change.
• I updated/added relevant documentation (doc comments with ///).
• I signed the CLA.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[monsieurtanuki]

My 2 cents: probably this has been discussed before, but using apply() instead of commit() seems to be the best practice on Android (and you wouldn't have to bother about the threads). Unless you need the return code of the operation (very unlikely I guess). It looks like the iOS version always returns true anyway.

[stuartmorgan-g]

Thanks for the contribution! This certainly looks good; it looks like the review missed that it wasn't using an actual queue when making it an instance variable rather than a one-off.

However, we should get a test in place for this in the PR to ensure that it doesn't regress again. There aren't currently native unit tests in place for it, but based on the discussion in the issue it seems like an integration tests that does two sets in row than waits for both to succeed should be able to catch this.

If you can add it that works be great, otherwise I can take a look tomorrow. This will also need a version and CHANGELOG update.

[stuartmorgan-g]

Unless you need the return code of the operation (very unlikely I guess).

It's definitely not unlikely; it's used 100% of the time (to send back in the response).

It looks like the iOS version always returns true anyway.

There is no mechanism on iOS for NSUserDefaults to fail and that to be communicated to the caller, so on iOS one has to trust that it always succeeds. That's not the case on Android. The existence of one behavior on one platform doesn't mean that we should treat a different API on a different platform as if it had that behavior.

[stuartmorgan-g]

Test added; I verified that it triggered the bug for me locally without the fix, so it would catch the issue at least sometimes; I don't see a way to test it deterministically unfortunately.

[mvanbeusekom]

LGTM!

[monsieurtanuki]

@stuartmorgan I guess that if you set a value 20 times in a loop without await there's a fair chance that you get a concurrent exception - I mean with the old version of the code.

Besides, I know that you return the appropriate value on Android, I just doubt that many developers check the return code from setxxx calls in SharedPreferences.

[stuartmorgan-g]

Besides, I know that you return the appropriate value on Android, I just doubt that many developers check the return code from setxxx calls in SharedPreferences.

There is a fundamental difference between an individual developer choosing not to check the return code, and the plugin making it impossible for any developer to check the return code.

[monsieurtanuki]

@stuartmorgan As a former Android java dev (where apply() is the officially recommended method to use) I don't agree but I understand your point. I don't think there's a 100% good solution anyway. Have a nice day!

[stuartmorgan-g]

@stuartmorgan As a former Android java dev (where apply() is the officially recommended method to use)

Please provide sources for that, because it's not what I'm seeing:

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

If you don't care about the return value and you're using this from your application's main thread, consider using apply() instead.

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

it's safe to replace any instance of commit() with apply() if you were already ignoring the return value.

[monsieurtanuki]

I have nothing to add to your excerpts about Android. I guess that the notion of synchronous calls on the main thread on Android (probably one of the first things you learn to avoid on Android java) (and where it's suggested to consider using apply()) is not that relevant on flutter. And I must say I'm feeling much better developing on flutter than on Android java (though obviously I sometimes still think like an Android java dev).

You consider that the developer should be given the possibility to check synchronously the return code, it's safer, fair enough.
The fact that the behavior is different on other platforms (ios and web always returning true) is problematic.

I have absolutely no idea about how I could gracefully add the return code checks to all my set calls, and what I should do in case the return code is false (I would probably be in big trouble if even SharedPreferences does not work).
I admit that I would enjoy being able to check the return code for some important commits (important data, large data).

That's all I have to say about that.