make AsyncStorage serially execute requests (#18522)

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

Author: Daniel Cochran

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

@@ -7,10 +7,13 @@
 
 package com.facebook.react.modules.storage;
 
+import java.util.ArrayDeque;
 import java.util.HashSet;
+import java.util.concurrent.Executor;
 
 import android.database.Cursor;
 import android.database.sqlite.SQLiteStatement;
+import android.os.AsyncTask;
 
 import com.facebook.common.logging.FLog;
 import com.facebook.react.bridge.Arguments;
@@ -23,6 +26,7 @@
 import com.facebook.react.bridge.WritableArray;
 import com.facebook.react.bridge.WritableMap;
 import com.facebook.react.common.ReactConstants;
+import com.facebook.react.common.annotations.VisibleForTesting;
 import com.facebook.react.module.annotations.ReactModule;
 import com.facebook.react.modules.common.ModuleDataCleaner;
 
@@ -43,8 +47,47 @@ public final class AsyncStorageModule
   private ReactDatabaseSupplier mReactDatabaseSupplier;
   private boolean mShuttingDown = false;
 
+  // Adapted from https://android.googlesource.com/platform/frameworks/base.git/+/1488a3a19d4681a41fb45570c15e14d99db1cb66/core/java/android/os/AsyncTask.java#237
+  private class SerialExecutor implements Executor {
+    private final ArrayDeque<Runnable> mTasks = new ArrayDeque<Runnable>();
+    private Runnable mActive;
+    private final Executor executor;
+
+    SerialExecutor(Executor executor) {
+      this.executor = executor;
+    }
+
+    public synchronized void execute(final Runnable r) {
+      mTasks.offer(new Runnable() {
+        public void run() {
+          try {
+            r.run();
+          } finally {
+            scheduleNext();
+          }
+        }
+      });
+      if (mActive == null) {
+        scheduleNext();
+      }
+    }
+    synchronized void scheduleNext() {
+      if ((mActive = mTasks.poll()) != null) {
+        executor.execute(mActive);
+      }
+    }
+  }
+
+  private final SerialExecutor executor;
+  
   public AsyncStorageModule(ReactApplicationContext reactContext) {
+    this(reactContext, AsyncTask.THREAD_POOL_EXECUTOR);
+  }
+
+  @VisibleForTesting
+  AsyncStorageModule(ReactApplicationContext reactContext, Executor executor) {
     super(reactContext);
+    this.executor = new SerialExecutor(executor);
     mReactDatabaseSupplier = ReactDatabaseSupplier.getInstance(reactContext);
   }
 
@@ -141,7 +184,7 @@ protected void doInBackgroundGuarded(Void... params) {
 
         callback.invoke(null, data);
       }
-    }.execute();
+    }.executeOnExecutor(executor);
   }
 
   /**
@@ -208,7 +251,7 @@ protected void doInBackgroundGuarded(Void... params) {
           callback.invoke();
         }
       }
-    }.execute();
+    }.executeOnExecutor(executor);
   }
 
   /**
@@ -259,7 +302,7 @@ protected void doInBackgroundGuarded(Void... params) {
           callback.invoke();
         }
       }
-    }.execute();
+    }.executeOnExecutor(executor);
   }
 
   /**
@@ -322,7 +365,7 @@ protected void doInBackgroundGuarded(Void... params) {
           callback.invoke();
         }
       }
-    }.execute();
+    }.executeOnExecutor(executor);
   }
 
   /**
@@ -345,7 +388,7 @@ protected void doInBackgroundGuarded(Void... params) {
           callback.invoke(AsyncStorageErrorUtil.getError(null, e.getMessage()));
         }
       }
-    }.execute();
+    }.executeOnExecutor(executor);
   }
 
   /**
@@ -379,7 +422,7 @@ protected void doInBackgroundGuarded(Void... params) {
         }
         callback.invoke(null, data);
       }
-    }.execute();
+    }.executeOnExecutor(executor);
   }
 
   /**

ReactAndroid/src/test/java/com/facebook/react/modules/storage/AsyncStorageModuleTest.java

@@ -10,13 +10,16 @@
 import java.util.Arrays;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.concurrent.Executor;
 
 import android.app.Activity;
 import android.content.Context;
 import android.content.ContextWrapper;
+import android.os.AsyncTask;
 
 import com.facebook.react.bridge.Arguments;
 import com.facebook.react.bridge.Callback;
+import com.facebook.react.bridge.GuardedAsyncTask;
 import com.facebook.react.bridge.ReactApplicationContext;
 import com.facebook.react.bridge.ReactTestHelper;
 import com.facebook.react.bridge.JavaOnlyArray;
@@ -35,14 +38,15 @@
 import org.mockito.Mockito;
 import org.mockito.invocation.InvocationOnMock;
 import org.mockito.stubbing.Answer;
+import org.mockito.verification.VerificationMode;
 import org.powermock.api.mockito.PowerMockito;
 import org.powermock.core.classloader.annotations.PrepareForTest;
 import org.powermock.core.classloader.annotations.PowerMockIgnore;
 import org.powermock.modules.junit4.rule.PowerMockRule;
 import org.robolectric.RuntimeEnvironment;
 import org.robolectric.Robolectric;
 import org.robolectric.RobolectricTestRunner;
-import org.robolectric.annotation.Config;
+import org.robolectric.util.concurrent.RoboExecutorService;
 
 import static org.mockito.Mockito.mock;
 import static org.fest.assertions.api.Assertions.assertThat;
@@ -81,7 +85,10 @@ public Object answer(InvocationOnMock invocation) throws Throwable {
      });
 
     // don't use Robolectric before initializing mocks
-    mStorage = new AsyncStorageModule(ReactTestHelper.createCatalystContextForTest());
+    mStorage = new AsyncStorageModule(
+        ReactTestHelper.createCatalystContextForTest(),
+        new RoboExecutorService()
+    );
     mEmptyArray = new JavaOnlyArray();
   }
 
@@ -104,15 +111,15 @@ public void testMultiSetMultiGet() {
 
     Callback setCallback = mock(Callback.class);
     mStorage.multiSet(keyValues, setCallback);
-    Mockito.verify(setCallback, Mockito.times(1)).invoke();
+    verify(setCallback, Mockito.times(1)).invoke();
 
     JavaOnlyArray keys = new JavaOnlyArray();
     keys.pushString(key1);
     keys.pushString(key2);
 
     Callback getCallback = mock(Callback.class);
     mStorage.multiGet(keys, getCallback);
-    Mockito.verify(getCallback, Mockito.times(1)).invoke(null, keyValues);
+    verify(getCallback, Mockito.times(1)).invoke(null, keyValues);
 
     keys.pushString(fakeKey);
     JavaOnlyArray row3 = new JavaOnlyArray();
@@ -122,7 +129,7 @@ public void testMultiSetMultiGet() {
 
     Callback getCallback2 = mock(Callback.class);
     mStorage.multiGet(keys, getCallback2);
-    Mockito.verify(getCallback2, Mockito.times(1)).invoke(null, keyValues);
+    verify(getCallback2, Mockito.times(1)).invoke(null, keyValues);
   }
 
   @Test
@@ -143,22 +150,22 @@ public void testMultiRemove() {
 
     Callback getCallback = mock(Callback.class);
     mStorage.multiRemove(keys, getCallback);
-    Mockito.verify(getCallback, Mockito.times(1)).invoke();
+    verify(getCallback, Mockito.times(1)).invoke();
 
     Callback getAllCallback = mock(Callback.class);
     mStorage.getAllKeys(getAllCallback);
-    Mockito.verify(getAllCallback, Mockito.times(1)).invoke(null, mEmptyArray);
+    verify(getAllCallback, Mockito.times(1)).invoke(null, mEmptyArray);
 
     mStorage.multiSet(keyValues, mock(Callback.class));
 
     keys.pushString("fakeKey");
     Callback getCallback2 = mock(Callback.class);
     mStorage.multiRemove(keys, getCallback2);
-    Mockito.verify(getCallback2, Mockito.times(1)).invoke();
+    verify(getCallback2, Mockito.times(1)).invoke();
 
     Callback getAllCallback2 = mock(Callback.class);
     mStorage.getAllKeys(getAllCallback2);
-    Mockito.verify(getAllCallback2, Mockito.times(1)).invoke(null, mEmptyArray);
+    verify(getAllCallback2, Mockito.times(1)).invoke(null, mEmptyArray);
   }
 
   @Test
@@ -175,7 +182,7 @@ public void testMultiMerge() throws Exception {
     {
       Callback callback = mock(Callback.class);
       mStorage.multiGet(getArray(mergeKey), callback);
-      Mockito.verify(callback, Mockito.times(1))
+      verify(callback, Mockito.times(1))
           .invoke(null, JavaOnlyArray.of(getArray(mergeKey, value.toString())));
     }
 
@@ -200,7 +207,7 @@ public void testMultiMerge() throws Exception {
     value.put("foo2", createJSONObject("key1", "val3", "key2", "val2"));
     Callback callback = mock(Callback.class);
     mStorage.multiGet(getArray(mergeKey), callback);
-    Mockito.verify(callback, Mockito.times(1))
+    verify(callback, Mockito.times(1))
         .invoke(null, JavaOnlyArray.of(getArray(mergeKey, value.toString())));
   }
 
@@ -219,18 +226,18 @@ public void testGetAllKeys() {
 
     Callback getAllCallback = mock(Callback.class);
     mStorage.getAllKeys(getAllCallback);
-    Mockito.verify(getAllCallback, Mockito.times(1)).invoke(null, storedKeys);
+    verify(getAllCallback, Mockito.times(1)).invoke(null, storedKeys);
 
     Callback getAllCallback2 = mock(Callback.class);
     mStorage.multiRemove(getArray(keys[0]), mock(Callback.class));
 
     mStorage.getAllKeys(getAllCallback2);
-    Mockito.verify(getAllCallback2, Mockito.times(1)).invoke(null, getArray(keys[1]));
+    verify(getAllCallback2, Mockito.times(1)).invoke(null, getArray(keys[1]));
 
     mStorage.multiRemove(getArray(keys[1]), mock(Callback.class));
     Callback getAllCallback3 = mock(Callback.class);
     mStorage.getAllKeys(getAllCallback3);
-    Mockito.verify(getAllCallback3, Mockito.times(1)).invoke(null, mEmptyArray);
+    verify(getAllCallback3, Mockito.times(1)).invoke(null, mEmptyArray);
   }
 
   @Test
@@ -242,11 +249,11 @@ public void testClear() {
 
     Callback clearCallback2 = mock(Callback.class);
     mStorage.clear(clearCallback2);
-    Mockito.verify(clearCallback2, Mockito.times(1)).invoke();
+    verify(clearCallback2, Mockito.times(1)).invoke();
 
     Callback getAllCallback2 = mock(Callback.class);
     mStorage.getAllKeys(getAllCallback2);
-    Mockito.verify(getAllCallback2, Mockito.times(1)).invoke(null, mEmptyArray);
+    verify(getAllCallback2, Mockito.times(1)).invoke(null, mEmptyArray);
   }
 
   @Test
@@ -339,4 +346,14 @@ private JavaOnlyArray getArray(String... values) {
     }
     return array;
   }
+
+  private static void waitForAsync() {
+    Robolectric.flushForegroundThreadScheduler();
+    Robolectric.flushBackgroundThreadScheduler();
+  }
+
+  private static <T> T verify(T mock, VerificationMode mode) {
+    waitForAsync();
+    return Mockito.verify(mock, mode);
+  }
 }