make AsyncStorage serially execute requests #18522
Conversation
react-native-bot
added
the
Missing Changelog
facebook-github-bot
added
the
CLA Signed
|
is this different from |
|
@chirag04 nope, it's literally copied and pasted from there: There's just no way to import it, so I copied and pasted it and left a note in the file. |
|
@chirag04 can you clarify what your question is? The SerialExecutor class I copied in is the same as the one provided by Android's AsyncTask class. However, my PR instantiates an instance of SerialExecutor for use with AsyncStorage tasks, because somewhere else in react-native, the shared SerialExecutor class instance is getting blocked when remote debugging. Because the SerialExecutor class is private in AsyncTask, there's no way to import it, hence the copy and paste. |
Yes but an instance of the SerialExecutor class is public(SERIAL_EXECUTOR). So my question was why not use |
|
@chirag04 Correct, that's the default, and it's the SerialExecutor instance that's blocked elsewhere in RN. Using that would be identical to the existing behavior. This PR uses its own SerialExecutor instance so it does not get blocked by something else in the codebase. And yes, this does not get at the root cause, which is that somewhere is posting a task which hangs indefinitely. Still, I would argue landing this is pretty critical to dev UX, if you take a look at the comments / reactions to #14101, you can see how much it's impacting people. |
|
The android CI failure is unrelated: |
|
fwiw this change makes Android match iOS behavior too: static dispatch_queue_t RCTGetMethodQueue()
{
// ...
queue = dispatch_queue_create("com.facebook.react.AsyncLocalStorageQueue", DISPATCH_QUEUE_SERIAL);
// ...
}iOS uses a separate serial queue to execute AsyncStorage tasks. Android using the default serial AsyncTask queue means all other serial tasks can block AsyncStorage tasks on Android unlike iOS. |
|
Just tested this, works for me. |
|
@javache @janicduplessis to start with. @hramos do you know anyone at FB right now involved in Android part of things? |
This comment has been minimized.
This comment has been minimized.
|
@dannycochran I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project. |
|
Not sure what the bot is talking about. The file dates to the original release of RN in 2015, and the file still exists: |
|
@facebook-github-bot @janicduplessis requesting a review here. This is still a relevant issue affecting pretty much every Android developer. It is now the 6th most commented open issue, up two spots from three weeks ago :) |
|
Sorry about the delay reviewing this. Would it be possible to use newSingleThreadExecutor https://developer.android.com/reference/java/util/concurrent/Executors.html#newSingleThreadExecutor() instead of copying that code from AsyncTask. cc @mdvacca |
|
@janicduplessis That would create a new thread instead of reusing one from the pool that backs AsyncTask, but yeah you could do that. |
|
My app has this error when try to setItem in wrong format, like JSON without stringify. |
|
@dannycochran would love to see this moved along as the issue is now locked and it's a growing problem for folks. |
|
The bugs which this PR resolves, makes it really diffcult to debug apps on Android. The changes seem small and incremental. Wondering what's the hold up for the reviewers... |
This comment has been minimized.
This comment has been minimized.
|
@gaearon This is another critical issue that impacts us. We had to fork the entire AsyncStorage module into our own codebase so we could apply this patch and set canOverrideExistingModule() to true. This PR has been open now for for 3 months. It'd be really awesome if you could help move this along, you had mentioned folks should ping you if PRs and issues were not being addressed. |
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 facebook#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. facebook#16905 [Android] [BUGFIX] [AsyncStorage] - Fix AsyncStorage causing remote debugger to hang indefinitely. Closes facebook#18522 Differential Revision: D8624088 fbshipit-source-id: 2a599fbca270a4826063cd93be8eaf796f34f57a
|
I was able to reproduce on Circle CI the same failures we observed internally. See https://circleci.com/gh/hramos/react-native/3233?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link, an example: |
hramos
removed
Import Failed
Import Started
|
The Java patch unfortunately didn't help me. For those that are stuck, I wrote this as a workaround. Mapping getItem() / setItem() / removeItem() to an array of Promises got the job done for me. Code: export default class AsyncStorageSupplement {
static multiGet(keys) {
return Promise.all(
keys.map(key => AsyncStorage.getItem(key))
)
}
static multiRemove(keys) {
return Promise.all(
keys.map(key => AsyncStorage.removeItem(key))
)
}
static multiSet(pairs) {
return Promise.all(
pairs.map(pair => AsyncStorage.setItem(pair[0], pair[1]))
)
}
} |
|
@mcavaliere your workaround worked for me. Is the problem releted only to "multi" prefixed methods??? |
|
@hramos Looking at the CI failure (https://circleci.com/gh/facebook/react-native/38275?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link), the error looks completely unrelated to this PR: But the PR does not touch any of the files involved—it just changes one Java file. I would have to guess that the failure was due to something else breaking the build, and if master passes, the PR should too. Could you maybe re-run CI and see if it passes |
|
@haggholm |
|
@hramos I have to admit I’m kind of crap with git and not familiar with rebase and such, but (a) it looks like the OP has not been active in a while, and (b) I do really want to see this make it in. In case it helps, I created a mirror PR for CI: #20258 Anything I can do to help get this change in, please let me know. |
|
@hramos I had to make some changes to make tests pass; once the earlier (irrelevant) errors were out of the way, the |
|
Thanks @haggholm, I'll give @dannycochran a chance to take in your changes from #20258 and see if we can get this merged. |
|
@hramos Can this be merged now? It looks like the only failing test is iOS E2E, which can hardly depend on changes to |
facebook-github-bot
added
the
Import Started
There was a problem hiding this comment.
hramos is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
This pull request was closed by Daniel Cochran in 1b09bd7. Once this commit is added to a release, you will see the corresponding version tag below the description at 1b09bd7. If the commit has a single |
|
@dannycochran thank you for investigating and coming up with solution to the common issue. Could you please look at my solution (#20386). IMHO, using dedicated thread executor to access SQLite will improve performance because AsyncTask.THREAD_POOL_EXECUTOR is shared and might be busy with other AsyncTask tasks. Also it allows you to provide custom executor if you want concurrency etc. |
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 facebook#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. facebook#16905 [Android] [BUGFIX] [AsyncStorage] - Fix AsyncStorage causing remote debugger to hang indefinitely. Pull Request resolved: facebook#18522 Differential Revision: D8624088 Pulled By: hramos fbshipit-source-id: a1d2e3458d98467845cb34ac73f2aafaaa15ace2
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 facebook#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. facebook#16905 [Android] [BUGFIX] [AsyncStorage] - Fix AsyncStorage causing remote debugger to hang indefinitely. Pull Request resolved: facebook#18522 Differential Revision: D8624088 Pulled By: hramos fbshipit-source-id: a1d2e3458d98467845cb34ac73f2aafaaa15ace2
hramos
removed
Import Started
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. facebook/react-native#16905 [Android] [BUGFIX] [AsyncStorage] - Fix AsyncStorage causing remote debugger to hang indefinitely. Pull Request resolved: facebook/react-native#18522 Differential Revision: D8624088 Pulled By: hramos fbshipit-source-id: a1d2e3458d98467845cb34ac73f2aafaaa15ace2
Motivation
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
Test Plan
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:
Related PRs
No documentation change is required.
#16905
Release Notes
[Android] [BUGFIX] [AsyncStorage] - Fix AsyncStorage causing remote debugger to hang indefinitely.