getEntityRecords resolver crashes on Android when fetching multiple items #29928
Labels
Mobile App - i.e. Android or iOS
Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change)
[Type] Bug
An existing feature does not function as intended
Comments
fluiddot
added
[Type] Bug
|
This crash has been prevented with a workaround in this PR. |
|
@youknowriad I was wondering if you could help me by providing some feedback regarding the solution "Prevent recursion when executing actions" as I saw you already worked in the Thank you very much for the help 🙇 ! |
|
Hi There! reading the report above, it's not clear to me where the recursion happens (where the depth increases), those "START" / "FINISH" resolution actions don't trigger recursion as far as I know? |
Yeah, the "START" / "FINISH" resolution doesn't trigger other actions. The problem is more related to how the unhandled action controls are executed in Let me show it better with a stacktrace that I took adding a breakpoint when the last item in the Stacktrace (with comments)Full stacktrace (1420 lines)As you can see in the above stacktrace, it gets bigger for each item in the loop, unfortunately this on Android ends up exceeding the call stack size. |
|
So if I'm understanding properly, there's no recursion, it's just that Android has a small stack size right?
|
Yes, probably I shouldn't have used the word "recursion" because it's a bit misleading since it's not actually happening (I'll rename it). EDIT: I've just renamed the solution "Prevent recursion when executing actions" to "Prevent the call stack to grow excessively when executing routines".
I was wondering how other libraries handle this case, maybe we can check Redux-saga in case it gives us some ideas.
This is one of the potential solutions I added to the issue, but it's neither easy to do because the configuration is not exposed so we should build a custom version. |
|
I tried to reproduce the growing stack with a minimalistic example that also dispatches a lot of actions inside a generator: const { dispatch, registerStore } = require( '@wordpress/data' );
registerStore( 'recurse', {
reducer: ( state = 0, action ) => state,
actions: {
curse: function* () {
for ( let i = 0; i < 100; i++ ) {
yield { type: 'CURSE' };
console.trace();
}
},
},
} );
dispatch( 'recurse' ).curse();But running this doesn't show a stack growing. Each of the 100 So, what does the code that dispatches |
This seems to be some quirk in how It happens because function* curse( times ) {
for ( let i = 0; i < times; i++ ) {
yield `curse ${ i }`;
}
}
function process( value, next ) {
console.log( value );
next();
}
function rungen( iter ) {
function next() {
const n = iter.next();
if ( ! n.done ) {
process( n.value, next );
}
}
next();
}
rungen( curse( 10000 ) );Running this program will terminate with stack overflow after 5652 iterations. I'm afraid this is a fundamental shortcoming in I hope this shortcoming will be removed once we migrate the |
That's exactly what I was experiencing, unfortunately on Android the threshold for triggering the stack overflow is quite lower (around 1200 iterations).
Yeah, actually in the case I exposed in this issue, the problem comes from a
Oh that's very interesting, I didn't know that we were migrating it. Thank you very much @jsnajdr for checking this and providing the example, it's very descriptive 🙇 ! |
|
A brief look at Redux Saga shows me that they have the same problem. They also have a This is probably the fate of all generator-based effect runtimes that try to be sync and async at the same time. It could be solved if JavaScript could do tail call optimizations, but most engines don't. |
I also wanted to check how Redux Saga handles this so thanks for exploring it beforehand 🙇 . I'm surprised that they have the same problem 😞 . Thinking out loud, I wonder if a potential solution would be to use microtasks although it's still not fully supported. Ideally, as you commented, this problem would be addressed if the JS engine do tail call optimizations but at least with this option we would defer the execution of |
Queuing a microtask is already possible with |
I wonder, the list of defined reusable blocks is not related to the post/page currently open in the editor so, sounds to me like data that we should fetch in the parent mobile app. Like, fetch it like we fetch other site data and keep it in local DB. The mobile apps act as a glorified cache anyway so, maybe populating the reusable blocks cache can be moved there. It won't be a "real" solution to the call stack depth limit problem, but still makes sense architecturally. Am I perhaps missing something? 🤔 |
|
A reasonable solution would be to introduce batched actions Such batched actions would also prevent firing too many change events. Now, when receiving a page of 100 items, the store fires 200+ change events. |
Yes, the list of defined reusable blocks is not related to the post/page but to the site. This is an interesting solution I didn't consider, my approach for the reusable blocks was to follow the same flow as we have in the web version. However if architecturally makes more sense to rely on the parent mobile app for fetching data, this is a solution that I definitely I'd like to explore. Thanks @hypest for pointing this solution out 🙇 . |
This is a great idea @jsnajdr! Add the batched actions ( Regarding the problem with the reusable blocks, I'm going to consider both options (the one about moving the logic to the mobile app and this one). So I’m going to keep this issue open because this would still be a potential problem if we use |
42 tasks
7 tasks
|
👋 Reviving this issue: I worked on adding the batched actions as @jsnajdr pointed in a previous comment. I created a draft PR that adds these new actions and applies them into the Before proceeding, I'd appreciate it if someone could do an initial review just in case I'm overlooking something important. @youknowriad @jsnajdr could you help me with this? If not please let me know who would be the right people to assign as reviewers, thanks 🙇 ! |
|
I'm closing this issue since it has been fixed via #31005 🎊 . |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Description
When the editor is opened, the reusable blocks are fetched from the site via this selector in the
useBlockEditorSettingshook.The selector used in this case is
getEntityRecordsthat has a resolver associated which executes the fetch request and updates the state with the response.This resolver apart from fetching, it also updates the entity cache for each item, this way if the selector
getEntityRecordis called for getting one of the reusable blocks, it will get it from the cache instead of doing a fetch request as we already fetched that item ingetEntityRecords.The cache update is done by calling the actions
START_RESOLUTIONandFINISH_RESOLUTIONfor each item, these actions are the same that would be triggered when the item is fetched via thegetEntityRecordresolver (related code). Initially this shouldn't be a problem but I realised that each action, as well as any other routine (control or promise), called from the resolver is nested which could end up with a long call stack.As an example, in the following logs you can see how many routines are nested when fetching 30 reusable blocks.
These logs have been obtained by adding logs in the function (
createRuntime) of the Redux middleware for generator coroutines package, that handles the routines execution..Logs
The log format is
<ROUTINE_TYPE> => <CALLSTACK_DEPTH> <ROUTINE_ARGS>ROUTINE_TYPE: There're three types, control, promise and unhandledActionControl (more info here).CALLSTACK_DEPTH: The depth of the routine in the call stack.ROUTINE_ARGS: The arguments of the routine.As you can see depending on the number of items fetched, more
START_RESOLUTIONandFINISH_RESOLUTIONactions are executed. The problem here is that on Android when there's more than 27 items, this exceeds the maximum call stack size, in the following error stacktrace you can see the error.Error stacktrace
This error is not happening on the web and iOS version because most likely the threshold of the call stack size is higher than on Android. On Android we use Hermes (a JavaScript engine optimized for React Native), the code related to this value is located here.
Solution
Add batched actions (
START_RESOLUTIONSandFINISH_RESOLUTIONS)Instead of yield a start and finish action for each item, we would batch them and yield only two actions each time a page of results is received.
These actions would accept an array of args arrays to mark as resolving/resolved. Such batched actions would also prevent firing too many change events. Now, when receiving a page of 100 items, the store fires 200+ change events.
Thanks @jsnajdr for proposing this solution in this comment.
Other potential solutions
Prevent the call stack to grow excessively when executing routines
I was thinking about the option of changing the way that next routines are called when running an unhandled action controls. Instead of calling the function we could use Microtasks.
Another option would be to use tasks directly and wrapping the call with
setTimeout.I'm not very familiar with the Redux middleware for generator coroutines package so I'd appreciate feedback for this option.
Increase native stack depth on Hermes
This solution would also work but it would imply to maintain and build our version of Hermes which could be overkilling. I found out some documentation about this.
Prevent getEntityRecords resolver to update the entity cache for each item
This would also work because we would ensure that just a small number of routines are executed. However this would be a temporary workaround because if in the future we have another resolver that executes a lot of routines, we would have a new crash.
For this option we could use the
_fieldsquery parameter or add a condition in the resolver that skips this part if the platform is native.Step-by-step reproduction instructions
Expected behaviour
The editor shouldn't crash when it's opened.
Actual behaviour
The editor crashes when is opened.
Screenshots or screen recording (optional)
N/A
WordPress information
Device information
The text was updated successfully, but these errors were encountered: