From 1b09bd7fba92431d63d2cecb83565491e91db396 Mon Sep 17 00:00:00 2001 From: Daniel Cochran Date: Mon, 30 Jul 2018 12:00:35 -0700 Subject: [PATCH] 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. https://github.com/facebook/react-native/pull/16905 [Android] [BUGFIX] [AsyncStorage] - Fix AsyncStorage causing remote debugger to hang indefinitely. Pull Request resolved: https://github.com/facebook/react-native/pull/18522 Differential Revision: D8624088 Pulled By: hramos fbshipit-source-id: a1d2e3458d98467845cb34ac73f2aafaaa15ace2 --- .../modules/storage/AsyncStorageModule.java | 55 +++++++++++++++++-- .../storage/AsyncStorageModuleTest.java | 49 +++++++++++------ 2 files changed, 82 insertions(+), 22 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/modules/storage/AsyncStorageModule.java b/ReactAndroid/src/main/java/com/facebook/react/modules/storage/AsyncStorageModule.java index c9fc96e57..6aa355f27 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/modules/storage/AsyncStorageModule.java +++ b/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.ReadableArray; 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 mTasks = new ArrayDeque(); + 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 @@ public final class AsyncStorageModule callback.invoke(null, data); } - }.execute(); + }.executeOnExecutor(executor); } /** @@ -208,7 +251,7 @@ public final class AsyncStorageModule callback.invoke(); } } - }.execute(); + }.executeOnExecutor(executor); } /** @@ -259,7 +302,7 @@ public final class AsyncStorageModule callback.invoke(); } } - }.execute(); + }.executeOnExecutor(executor); } /** @@ -322,7 +365,7 @@ public final class AsyncStorageModule callback.invoke(); } } - }.execute(); + }.executeOnExecutor(executor); } /** @@ -345,7 +388,7 @@ public final class AsyncStorageModule callback.invoke(AsyncStorageErrorUtil.getError(null, e.getMessage())); } } - }.execute(); + }.executeOnExecutor(executor); } /** @@ -379,7 +422,7 @@ public final class AsyncStorageModule } callback.invoke(null, data); } - }.execute(); + }.executeOnExecutor(executor); } /** diff --git a/ReactAndroid/src/test/java/com/facebook/react/modules/storage/AsyncStorageModuleTest.java b/ReactAndroid/src/test/java/com/facebook/react/modules/storage/AsyncStorageModuleTest.java index 472f9678f..559c8669b 100644 --- a/ReactAndroid/src/test/java/com/facebook/react/modules/storage/AsyncStorageModuleTest.java +++ b/ReactAndroid/src/test/java/com/facebook/react/modules/storage/AsyncStorageModuleTest.java @@ -10,13 +10,16 @@ package com.facebook.react.modules.storage; 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,6 +38,7 @@ import org.mockito.Mock; 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; @@ -42,7 +46,7 @@ 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 class AsyncStorageModuleTest { }); // don't use Robolectric before initializing mocks - mStorage = new AsyncStorageModule(ReactTestHelper.createCatalystContextForTest()); + mStorage = new AsyncStorageModule( + ReactTestHelper.createCatalystContextForTest(), + new RoboExecutorService() + ); mEmptyArray = new JavaOnlyArray(); } @@ -104,7 +111,7 @@ public class AsyncStorageModuleTest { 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); @@ -112,7 +119,7 @@ public class AsyncStorageModuleTest { 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 class AsyncStorageModuleTest { 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 class AsyncStorageModuleTest { 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 class AsyncStorageModuleTest { { 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 class AsyncStorageModuleTest { 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 class AsyncStorageModuleTest { 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 class AsyncStorageModuleTest { 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 @@ public class AsyncStorageModuleTest { } return array; } + + private static void waitForAsync() { + Robolectric.flushForegroundThreadScheduler(); + Robolectric.flushBackgroundThreadScheduler(); + } + + private static T verify(T mock, VerificationMode mode) { + waitForAsync(); + return Mockito.verify(mock, mode); + } }