Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

execute calls to AsyncStorageModule serially #20386

Closed
wants to merge 11 commits into from
Closed

execute calls to AsyncStorageModule serially #20386

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
0ae8e20
make calls to AsyncStorageModule serial
dulmandakh Jul 25, 2018
0227e95
shutdown executor on onCatalystInstanceDestroy, handle RuntimeException
dulmandakh Jul 30, 2018
0384323
AsyncStorageModuleTest works
dulmandakh Jul 30, 2018
e92a018
Merge branch 'master' into serial-asyncstorage
dulmandakh Jul 30, 2018
6463c07
merge master
dulmandakh Aug 22, 2018
b41c95a
change AsyncStorageModule constructor
dulmandakh Aug 22, 2018
92560ef
Merge branch 'master' into serial-asyncstorage
dulmandakh Aug 23, 2018
480e7f7
merge master
dulmandakh Sep 17, 2018
2c6b497
AsyncStorageModule.java: rename variable mExecutor -> executor
dulmandakh Sep 17, 2018
cde3ac5
AsyncStorageModuleTest.java: fix indent
dulmandakh Sep 17, 2018
9527119
Merge branch 'master' into serial-asyncstorage
dulmandakh Nov 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 39 additions & 20 deletions ReactAndroid/src/main/java/com/facebook/react/modules/storage/AsyncStorageModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/**
/*
Copy link
Contributor

@gengjiawen gengjiawen Jul 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why touch this line ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Android Studio was showing warning, so fixed it 😄

* Copyright (c) 2015-present, Facebook, Inc.
*
* This source code is licensed under the MIT license found in the
Expand All @@ -8,14 +8,16 @@
package com.facebook.react.modules.storage;

import java.util.HashSet;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;

import android.database.Cursor;
import android.database.sqlite.SQLiteStatement;

import com.facebook.common.logging.FLog;
import com.facebook.infer.annotation.Assertions;
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.ReactContextBaseJavaModule;
import com.facebook.react.bridge.ReactMethod;
Expand All @@ -42,10 +44,17 @@ public final class AsyncStorageModule

private ReactDatabaseSupplier mReactDatabaseSupplier;
private boolean mShuttingDown = false;
private ExecutorService mExecutor;

public AsyncStorageModule(ReactApplicationContext reactContext) {
super(reactContext);
mReactDatabaseSupplier = ReactDatabaseSupplier.getInstance(reactContext);
mExecutor = Executors.newSingleThreadExecutor();
}

public AsyncStorageModule(ReactApplicationContext reactContext, ExecutorService executor) {
this(reactContext);
mExecutor = executor;
}

@Override
Expand All @@ -61,6 +70,7 @@ public void initialize() {

@Override
public void onCatalystInstanceDestroy() {
mExecutor.shutdown();
mShuttingDown = true;
}

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

new GuardedAsyncTask<Void, Void>(getReactApplicationContext()) {
execute(new Runnable() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Override
protected void doInBackgroundGuarded(Void... params) {
public void run() {
if (!ensureDatabase()) {
callback.invoke(AsyncStorageErrorUtil.getDBError(null), null);
return;
Expand Down Expand Up @@ -141,7 +151,7 @@ protected void doInBackgroundGuarded(Void... params) {

callback.invoke(null, data);
}
}.execute();
});
}

/**
Expand All @@ -156,9 +166,9 @@ public void multiSet(final ReadableArray keyValueArray, final Callback callback)
return;
}

new GuardedAsyncTask<Void, Void>(getReactApplicationContext()) {
execute(new Runnable() {
@Override
protected void doInBackgroundGuarded(Void... params) {
public void run() {
if (!ensureDatabase()) {
callback.invoke(AsyncStorageErrorUtil.getDBError(null));
return;
Expand Down Expand Up @@ -208,7 +218,7 @@ protected void doInBackgroundGuarded(Void... params) {
callback.invoke();
}
}
}.execute();
});
}

/**
Expand All @@ -221,9 +231,9 @@ public void multiRemove(final ReadableArray keys, final Callback callback) {
return;
}

new GuardedAsyncTask<Void, Void>(getReactApplicationContext()) {
execute(new Runnable() {
@Override
protected void doInBackgroundGuarded(Void... params) {
public void run() {
if (!ensureDatabase()) {
callback.invoke(AsyncStorageErrorUtil.getDBError(null));
return;
Expand Down Expand Up @@ -259,7 +269,7 @@ protected void doInBackgroundGuarded(Void... params) {
callback.invoke();
}
}
}.execute();
});
}

/**
Expand All @@ -268,9 +278,9 @@ protected void doInBackgroundGuarded(Void... params) {
*/
@ReactMethod
public void multiMerge(final ReadableArray keyValueArray, final Callback callback) {
new GuardedAsyncTask<Void, Void>(getReactApplicationContext()) {
execute(new Runnable() {
@Override
protected void doInBackgroundGuarded(Void... params) {
public void run() {
if (!ensureDatabase()) {
callback.invoke(AsyncStorageErrorUtil.getDBError(null));
return;
Expand Down Expand Up @@ -322,17 +332,17 @@ protected void doInBackgroundGuarded(Void... params) {
callback.invoke();
}
}
}.execute();
});
}

/**
* Clears the database.
*/
@ReactMethod
public void clear(final Callback callback) {
new GuardedAsyncTask<Void, Void>(getReactApplicationContext()) {
execute(new Runnable() {
@Override
protected void doInBackgroundGuarded(Void... params) {
public void run() {
if (!mReactDatabaseSupplier.ensureDatabase()) {
callback.invoke(AsyncStorageErrorUtil.getDBError(null));
return;
Expand All @@ -345,17 +355,17 @@ protected void doInBackgroundGuarded(Void... params) {
callback.invoke(AsyncStorageErrorUtil.getError(null, e.getMessage()));
}
}
}.execute();
});
}

/**
* Returns an array with all keys from the database.
*/
@ReactMethod
public void getAllKeys(final Callback callback) {
new GuardedAsyncTask<Void, Void>(getReactApplicationContext()) {
execute(new Runnable() {
@Override
protected void doInBackgroundGuarded(Void... params) {
public void run() {
if (!ensureDatabase()) {
callback.invoke(AsyncStorageErrorUtil.getDBError(null), null);
return;
Expand All @@ -379,7 +389,7 @@ protected void doInBackgroundGuarded(Void... params) {
}
callback.invoke(null, data);
}
}.execute();
});
}

/**
Expand All @@ -388,4 +398,13 @@ protected void doInBackgroundGuarded(Void... params) {
private boolean ensureDatabase() {
return !mShuttingDown && mReactDatabaseSupplier.ensureDatabase();
}

private void execute(Runnable r) {
Assertions.assertNotNull(mExecutor);
try {
mExecutor.execute(r);
} catch (RuntimeException e) {
getReactApplicationContext().handleException(e);
}
}
}
13 changes: 2 additions & 11 deletions ReactAndroid/src/test/java/com/facebook/react/modules/storage/AsyncStorageModuleTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,11 @@
import java.util.HashMap;
import java.util.Map;

import android.app.Activity;
import android.content.Context;
import android.content.ContextWrapper;

import com.facebook.react.bridge.Arguments;
import com.facebook.react.bridge.Callback;
import com.facebook.react.bridge.ReactApplicationContext;
import com.facebook.react.bridge.ReactTestHelper;
import com.facebook.react.bridge.JavaOnlyArray;
import com.facebook.react.bridge.JavaOnlyMap;
import com.facebook.react.modules.storage.AsyncStorageModule;
import com.facebook.react.modules.storage.ReactDatabaseSupplier;

import org.json.JSONArray;
import org.json.JSONObject;
Expand All @@ -40,13 +33,11 @@
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;

/**
* Tests for {@link AsyncStorageModule}.
*/
Expand Down Expand Up @@ -81,7 +72,7 @@ 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();
}

Expand Down