execute calls to AsyncStorageModule serially #20386

dulmandakh · 2018-07-25T09:19:20Z

This PR creates single threaded executor in AsyncStorageModule and use it to execute calls serially, so that below code works as expected

await AsyncStorage.setItem('foo', 'bar');
await AsyncStorage.getItem('foo').then(console.log); // 'bar'
await AsyncStorage.getAllKeys().then(console.log); // ['foo']

Previously, it was using AsyncTask that executes tasks in its own thread pool, which may execute calls in parallel.

Since it uses its own executor, it don't need to wrap the executions in the AsyncTask.

Fixes #18372 #14101

Test Plan:

Before (it's inconsistent)

await AsyncStorage.setItem('foo', 'bar');
await AsyncStorage.getItem('foo').then(console.log); // null
await AsyncStorage.getAllKeys().then(console.log); // []

After

await AsyncStorage.setItem('foo', 'bar');
await AsyncStorage.getItem('foo').then(console.log); // 'bar'
await AsyncStorage.getAllKeys().then(console.log); // ['foo']

Release Notes:

Help reviewers and the release process by writing your own release notes. See below for an example.

[ANDROID] [BUGFIX] [AsyncStorage] - Execute calls serially

dulmandakh · 2018-07-25T09:21:38Z

@hramos @gengjiawen I have no experience of writing android tests, could you please help update tests for AsyncStorageModule?

gengjiawen · 2018-07-26T14:19:57Z

ReactAndroid/src/main/java/com/facebook/react/modules/storage/AsyncStorageModule.java

@@ -1,4 +1,4 @@
-/**
+/*


why touch this line ?

Android Studio was showing warning, so fixed it 😄

gengjiawen · 2018-07-26T14:21:17Z

I have little test experience on android test. Do you test your code in some way , like tested it on real device.

dulmandakh · 2018-07-26T17:02:36Z

I was thinking that example for AsyncStorage in RNTester app would be useful, but I develop in TypeScript and have no experience of Flow. The example must be trivial, enter a key and value to store and enter key to retrieve its value.

gengjiawen · 2018-07-26T23:51:39Z

I find flow difficulty to write too. I want it in plain js. And also current project has to many #FlowFixme.

mdvacca · 2018-07-29T23:10:07Z

ReactAndroid/src/main/java/com/facebook/react/modules/storage/AsyncStorageModule.java

@@ -83,9 +85,9 @@ public void multiGet(final ReadableArray keys, final Callback callback) {
      return;
    }

-    new GuardedAsyncTask<Void, Void>(getReactApplicationContext()) {
+    execute(new Runnable() {


What happens if an exception is thrown when executing the Runnable?

GuardedAsyncTask provided error management, see: https://github.com/facebook/react-native/blob/master/ReactAndroid/src/main/java/com/facebook/react/bridge/GuardedAsyncTask.java#L34

Could we use executeOnExecutor (https://developer.android.com/reference/android/os/AsyncTask.html#executeOnExecutor(java.util.concurrent.Executor,%20Params...)) instead of execute and keep using GuardedAsyncTask instead?

@janicduplessis got it and ready to push the change, but waiting for CI to become green again.

mdvacca · 2018-07-29T23:15:32Z

ReactAndroid/src/main/java/com/facebook/react/modules/storage/AsyncStorageModule.java

@@ -42,6 +43,7 @@

  private ReactDatabaseSupplier mReactDatabaseSupplier;
  private boolean mShuttingDown = false;
+  private Executor executor = Executors.newSingleThreadExecutor();


I'm not sure if adding a new executor is the best approach here, but If you are adding this you should override the onCatalystInstanceDestroy method and then shutdown the executor.
See:

react-native/ReactAndroid/src/main/java/com/facebook/react/bridge/NativeModule.java

Line 51 in 26684cf

void onCatalystInstanceDestroy();

dulmandakh · 2018-07-29T23:57:03Z

Thank you @mdvacca for review and suggestions. Could you please suggest other options to make serially execute calls than executor. I intentionally wrapped in private execute method so we can easily change execution method in the future. I'll change it to handle runtime exception, and shutdown executor.

dulmandakh · 2018-07-30T00:51:18Z

@mdvacca made changes as you suggested 😉

dulmandakh · 2018-07-30T12:16:17Z

ExecutorService in AsyncStorageModule is customizable, and test is passing. It'll fix many issues on Android, including me.

@mdvacca please review and merge

Summary: This patch is a bit of a hack job, but I'd argue it's necessary to dramatically improve the dev UX on Android devices. Somewhere in react-native, there's a shared SerialExecutor which AsyncStorage uses that is getting blocked, causing remote debugging to occasionally hang indefinitely for folks making AsyncStorage requests. This is frustrating from a dev UX perspective, and has persisted across several versions as far back as RN 0.44, and still remains on RN 0.54. The issue seems to only happen on Android > 7+, which is likely because the ThreadPoolExecutor behavior changed in this version: https://stackoverflow.com/questions/9654148/android-asynctask-threads-limits Fixes #14101 We've been using this patch in production for the past 4 months on our team by overriding the AsyncStorage native module. We use AsyncStorage extensively for offline data and caching. You can test by compiling this commit version into a test react native repository that is set to build from source: ```sh git clone https://github.com/dannycochran/react-native rnAsyncStorage cd rnAsyncStorage git checkout asyncStorage cd .. git clone https://github.com/dannycochran/asyncStorageTest yarn install cp -r ../rnAsyncStorage node_modules/react-native react-native run-android ``` No documentation change is required. #16905 [Android] [BUGFIX] [AsyncStorage] - Fix AsyncStorage causing remote debugger to hang indefinitely. Pull Request resolved: #18522 Differential Revision: D8624088 Pulled By: hramos fbshipit-source-id: a1d2e3458d98467845cb34ac73f2aafaaa15ace2

gengjiawen · 2018-08-03T15:31:45Z

Maybe add RNTester showcase too. Can we just add plain js in RNTester ? @hramos @janicduplessis

hramos · 2018-08-17T17:23:13Z

I'll wait for @mdvacca to provide feedback here. Also, looks like we have some conflicts.

hramos

Fix conflicts.

dulmandakh · 2018-08-23T14:28:54Z

@hramos @janicduplessis please review and merge. CI fails on E2E due to timeout. Thank you

hramos · 2018-08-23T17:35:53Z

CI looks good, still waiting on review from @mdvacca or other. I'm not the best person to review this.

dulmandakh · 2018-08-24T12:26:10Z

@mdvacca could you please review ASAP. I would like it to land in 0.57 if possible, because I'm planning to use it for my projects. Thank you

ahilles107 · 2018-08-28T13:27:12Z

Would be great to have it in 0.57!

facebook-github-bot

hramos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

cpojer · 2019-02-11T22:41:58Z

Hey @dulmandakh. We moved AsyncStorage to its own repo, it would be great if you could re-send your PR to that one: https://github.com/react-native-community/react-native-async-storage

make calls to AsyncStorageModule serial

0ae8e20

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 25, 2018

react-native-bot added ✅Release Notes labels Jul 25, 2018

dulmandakh added the Platform: Android Android applications. label Jul 25, 2018

gengjiawen reviewed Jul 26, 2018

View reviewed changes

mdvacca reviewed Jul 29, 2018

View reviewed changes

shutdown executor on onCatalystInstanceDestroy, handle RuntimeException

0227e95

dulmandakh added 2 commits July 30, 2018 19:45

AsyncStorageModuleTest works

0384323

Merge branch 'master' into serial-asyncstorage

e92a018

dulmandakh requested a review from hramos July 31, 2018 02:15

dulmandakh mentioned this pull request Aug 8, 2018

make AsyncStorage serially execute requests #18522

Closed

hramos suggested changes Aug 17, 2018

View reviewed changes

dulmandakh added 3 commits August 23, 2018 07:48

merge master

6463c07

change AsyncStorageModule constructor

b41c95a

Merge branch 'master' into serial-asyncstorage

92560ef

dulmandakh added 3 commits September 17, 2018 20:50

merge master

480e7f7

AsyncStorageModule.java: rename variable mExecutor -> executor

2c6b497

AsyncStorageModuleTest.java: fix indent

cde3ac5

facebook-github-bot reviewed Sep 26, 2018

View reviewed changes

dulmandakh closed this Nov 1, 2018

dulmandakh reopened this Nov 1, 2018

dulmandakh closed this Nov 2, 2018

dulmandakh reopened this Nov 2, 2018

Merge branch 'master' into serial-asyncstorage

9527119

dulmandakh closed this Nov 2, 2018

dulmandakh reopened this Nov 2, 2018

hramos added Core Team and removed Core Team labels Feb 1, 2019

cpojer closed this Feb 11, 2019

dulmandakh deleted the serial-asyncstorage branch October 15, 2019 05:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

execute calls to AsyncStorageModule serially #20386

execute calls to AsyncStorageModule serially #20386

dulmandakh commented Jul 25, 2018 •

edited

Loading

dulmandakh commented Jul 25, 2018

gengjiawen Jul 26, 2018 •

edited

Loading

dulmandakh Jul 30, 2018

gengjiawen commented Jul 26, 2018

dulmandakh commented Jul 26, 2018 via email

gengjiawen commented Jul 26, 2018

mdvacca Jul 29, 2018

janicduplessis Aug 22, 2018

dulmandakh Aug 23, 2018

mdvacca Jul 29, 2018

dulmandakh commented Jul 29, 2018 via email •

edited

Loading

dulmandakh commented Jul 30, 2018 •

edited

Loading

dulmandakh commented Jul 30, 2018

gengjiawen commented Aug 3, 2018

hramos commented Aug 17, 2018

hramos left a comment

dulmandakh commented Aug 23, 2018 •

edited

Loading

hramos commented Aug 23, 2018

dulmandakh commented Aug 24, 2018

ahilles107 commented Aug 28, 2018

facebook-github-bot left a comment

cpojer commented Feb 11, 2019

execute calls to AsyncStorageModule serially #20386

execute calls to AsyncStorageModule serially #20386

Conversation

dulmandakh commented Jul 25, 2018 • edited Loading

Test Plan:

Release Notes:

dulmandakh commented Jul 25, 2018

gengjiawen Jul 26, 2018 • edited Loading

Choose a reason for hiding this comment

dulmandakh Jul 30, 2018

Choose a reason for hiding this comment

gengjiawen commented Jul 26, 2018

dulmandakh commented Jul 26, 2018 via email

gengjiawen commented Jul 26, 2018

mdvacca Jul 29, 2018

Choose a reason for hiding this comment

janicduplessis Aug 22, 2018

Choose a reason for hiding this comment

dulmandakh Aug 23, 2018

Choose a reason for hiding this comment

mdvacca Jul 29, 2018

Choose a reason for hiding this comment

dulmandakh commented Jul 29, 2018 via email • edited Loading

dulmandakh commented Jul 30, 2018 • edited Loading

dulmandakh commented Jul 30, 2018

gengjiawen commented Aug 3, 2018

hramos commented Aug 17, 2018

hramos left a comment

Choose a reason for hiding this comment

dulmandakh commented Aug 23, 2018 • edited Loading

hramos commented Aug 23, 2018

dulmandakh commented Aug 24, 2018

ahilles107 commented Aug 28, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

cpojer commented Feb 11, 2019

dulmandakh commented Jul 25, 2018 •

edited

Loading

gengjiawen Jul 26, 2018 •

edited

Loading

dulmandakh commented Jul 29, 2018 via email •

edited

Loading

dulmandakh commented Jul 30, 2018 •

edited

Loading

dulmandakh commented Aug 23, 2018 •

edited

Loading