Many logics causes deep call stacks

[riston]

Issue description

Application with many logics(example 100) would cause deep RxJS "next" call stacks.

I have recreated the issue with one of the example, added 100 dummy logics which are not triggered (https://github.com/riston/redux-logic/blob/perf-logic/examples/async-fetch-proc-options/src/users/logic.js). Dispatching the "fetch" call(could be any other as well) which now causes really long call stack.

Here are the Javascript profiler screenshots to give a better overview:

Start and dispatch part, function with names would make debugging easier:

Fulfilled http request promise handling, calls actions and causes another "next" cascade.

Possible solution

• Reduce the number of logics in application, dynamic add/remove logics.
• Inner changes in redux-logic code, to reduce the "next" cascade?

Why is this problem?

Mobile Safari has smaller execution stack which causes silent failures in stream handling, also possible freezing, crashing and other unexpected behaviours.

[jeffbski]

Thanks for identifying this @riston. I can see if there are ways to improve the situation internally or like you said via dynamically adding and removing logic. Maybe there are other strategies that could be done to segment for stacks. I can also discuss with the RxJS team.

I guess the next obvious question is: has there been any real world failures in a redux-logic app due to this (on Mobile Safari or otherwise)? or is are you just doing due dilligence to anticipate potential problems?

It's certainly worth giving some thought either way. I just wanted to know how to prioritize it along with everything.

[askot]

Hi Jeff, thank you for quick response. I am working together with @riston and we are seeing this right now in our app.

Our app has grown to having over 180 logics, and with recent release (after adding bunch of code), we started seeing issues on IOS browsers. Basically, when dispatching an action that dispatches a lot of secondary actions, app would stop responding, usually, without an error.

Debugging with the logics monitor$ showed that dispatching still starts, but code logics is not triggered any more. And same thing can be reproduced on desktop Chrome, when running with smaller stack size (for example for Chrome Canary - /Applications/Google\ Chrome\ Canary.app/Contents/MacOS/Google\ Chrome\ Canary --js-flags="--stack-size 400").

[jeffbski]

Thanks for the update @askot. I'll take a look and see what we can do to help this situation. And thanks for the tip about how to reproduce in canary that may help.

[riston]

As @askot already mentioned this is quite problematic for mobile devices. In the first example only single action gets dispatched but when multiple actions get dispatched the call stack gets really wide.

Here is the same example as before but added one additional dispatch after the http client is fulfilled:

The click handler takes ~650ms:

The part when XHR state change takes in total ~1.2s:

I am not a RxJs expert and it's always hard to spot or debug cases like this, but do you have ideas which might cause this issue?

[jeffbski]

I tried running with Canary with limited stack as you mentioned

/Applications/Google\ Chrome\ Canary.app/Contents/MacOS/Google\ Chrome\ Canary --js-flags="--stack-size 400"

and ran your example with the 100 logics (https://github.com/riston/redux-logic/blob/perf-logic/examples/async-fetch-proc-options/src/users/logic.js)

Should I expect it to fail? (or just to show the stack depth?)

Do you have any other examples that will fail if I use Canary with a limited stack size? I'd love to be able to reproduce the error condition you are seeing.

[jeffbski]

I was thinking about the issue and I wonder if it could be solved by swapping out the logic stack as you change to different parts of your app? That way actions are only running through a subset of logic in each part of your app. I think that would keep the stack size down.

We could build extra functionality in redux-logic to help with this if this turns out to be a good approach, but you can get started trying that now just using the replaceLogic.

So what you could do is instead of creating one array of logic.

You could create different arrays of logic that you can switch to in different parts of your app.

So you might create a foo group and a bar group of logic and then as you navigate to that part of the app, you could switch to that group of logic.

const fooLogicGroup = [
  logic1,
  logic2,
  logic3
];

const barLogicGroup = [
  logic10,
  logic11,
  logic12
];

You use the logicMiddleware instance that you created with createLogicMiddleware. I like to attach it to the store after I create it store.logicMiddleware = logicMiddleware so that I have access to it where I need it.

// assuming you have stored your logicMiddleware instance on store
store.logicMiddleware.replaceLogic(fooLogicGroup);

// elsewhere
store.logicMiddleware.replaceLogic(barLogicGroup);

If you need to have some logic in both groups, that is fine, just add them in as necessary to each group.

When you use replaceLogic, only that group will be running going forward (though existing actions being processed in the old stack will complete) so new actions will only hit this group of logic.

So I think that would allow you to really segment down your app and limit the number of logic that need to work on your actions at any one time.

See if this will work for your app and let me know the results. If you have any problems let me know. If this seems like a workable solution but we need some additional functionality to help manage it, we can brainstorm on some ideas. Right now I just want to see if this gets you past your issue.

[riston]

Hello Jeff, thank you for a quick response. I was mistaken the application does have more than 180 logics added to the application. Grouping logics and adding these dynamically sounds a good idea, although in our case one group would still have 160+ logics which would be in the same group.

The test with limited stack size would simulate similar issues occurring on iOS Safari. It does not need to be Canary, Chrome also supports following arguments --js-flags="--stack-size 400". With 180 logics, the following example(https://github.com/riston/redux-logic/blob/perf-logic/examples/async-fetch-proc-options/src/users/logic.js) should have silent failures, the monitoring stream seems to work but other actions dispatching starts failing:

Trigger users fetch, the fulfilled is triggered but never processed:

configureStore.js:33 > {action: {…}, op: "top"}
configureStore.js:33 > {action: {…}, name: "L(USERS_FETCH)-0", op: "begin"}
configureStore.js:33 > {action: {…}, nextAction: {…}, name: "L(USERS_FETCH)-0", shouldProcess: true, op: "next"}
configureStore.js:33 > {action: {…}, dispAction: {…}, op: "dispatch"}
configureStore.js:33 > {action: {…}, op: "top"}
configureStore.js:33 > {action: {…}, name: "L(USERS_FETCH)-0", op: "end"}

Triggering now cancel, no process:

> {action: {…}, op: "top"}

Here all the logics get wrapped https://github.com/jeffbski/redux-logic/blob/master/src/createLogicMiddleware.js#L223 and iside the wrapping function https://github.com/jeffbski/redux-logic/blob/master/src/logicWrapper.js#L39 each logic would receive separate stream? When some action A triggers, all the logics X streams would start filtering and merge mapping? More logics there are, the heavier gets the stack?

[jeffbski]

Ok, I can look closely at that example to see what is occurring. Thanks.

As for https://github.com/jeffbski/redux-logic/blob/master/src/logicWrapper.js#L39 I am using a share() because all the logics do want to share the same rxjs stream. That allows later logics to intercept previous values since they feed through this pipeline.

However that brings up an idea. The most common use of logic is for the process hook and not all logic actually need to do any interception (validate or transform). It is probably a much smaller subset.

I was already pondering an internal restructuring of redux-logic that I thought would simplify the code and maybe it would help the situation in this way. I was planning on handling the interception and processing hooks in a separate fashion.

So if the most logic doesn't use interception (validate or transform), then those pure process logics doesn't need to see all of the previous actions. Logic that is purely a process only hook only needs to know when to fire with its action. So the interception pipeline depth would be much smaller than the total number of logics.

Then for the process hooks, they probably don't even need to be in the same pipeline. They just need to be triggered in a predictable order but otherwise they do their thing independently.

So for your logic use cases, how many of them use interception (validate or transform)?

If it is indeed a minority, then I think the new structure would work well in reducing the stack size.

Let me know what you think.

[riston]

So in our case, we have 100% pure process hook based logics we don't use the validation, transform hooks. Many of our logics use multi actions dispatching which causes really long call stack. The separation of handling interception and processing hooks seems a great idea 👍 .

[VitalyChe]

The idea sounds great but could you, please, share some example of the implementation! Thank you!

[jeffbski]

Great @riston that sounds like a winner then.

Yes @VitalyChe I can write or show a design of how it will work, that way we can all look at it before I dive into the code. Just in case there are other things to consider. The more eyes the better.

I'll get something up for review asap.

[jeffbski]

@askot @riston I have an experimental branch that I would like you to try to see if it helps with the limited call stack issue on mobile browsers.

I have uploaded it to a branch "add-process-delay"

So you should be able to install it into your app using

npm i -S "jeffbski/redux-logic#add-process-delay"

This should pull it directly from the github branch. I have pushed the build/dist files to this branch so they are already built and thus it should install just like it would from npm.

Once it installs it should list it as [email protected], so you will know you are using the updated version

npm ls redux-logic

Anyway, I'd like to see if this help with your limited call stack problem. Basically I just made a change so that it runs the process hook asynchronously which should result in a smaller call stack for the process side of things.

Let me know how this version works for you.

Thanks.

[riston]

Hello Jeff, I tested the Rx scheduler change on the async-fetch-proc-options example, the execution cycle seems to be faster compared to the previous result(~650ms), although the stack size would stay same.

[jeffbski]

@riston does it prevent the failures you were seeing?

I'm still planning on doing the refactor I mentioned, but I was hoping that this might buy me some time to not have to rush it.

[askot]

Hi, Jeff, thank you for you work.

Yes, this patch seems to work around the freezing issue, so that our app does not hang any more on iOS where it was reproducible before.

As a side effect it brings out some wrong assumptions we had made on running order of logics, breaking data loading and some other timing issues. So it could be a breaking change for other users as well.

Also, wanted to thank you for a great library, it has helped us a lot to organise redux code in a clear and meaningful way.

[erkiesken]

Hey @jeffbski,

I'm in the same team with @riston and @askot. For now we had to fork this library to fix our issues for good. Your above workaround only alleviates the problem a bit.

The main issue is that this code in applyLogic chains all the logics together in nested merged observables: https://github.com/jeffbski/redux-logic/blob/v0.12.3/src/createLogicMiddleware.js#L227

Also, the share call here doesn't actually share the original actionIn$/actionSrc$ stream: https://github.com/jeffbski/redux-logic/blob/v0.12.3/src/logicWrapper.js#L39

It only shares it for the first logic's nonMatchingAction$ and matchingAction$ streams. Next logic gets this merged stream etc etc: https://github.com/jeffbski/redux-logic/blob/v0.12.3/src/logicWrapper.js#L55

What I did in our fork was to forego (for now) the feature of validate being able to change the action for downstream middlewares/reducers. In my fork validate output only affects same logic process input action. This allowed me to make the actionSrc$ here (https://github.com/jeffbski/redux-logic/blob/v0.12.3/src/createLogicMiddleware.js#L50) be observeOn(asap), and unchain the applyLogic reducer. Now all actions will be handled async (with asap scheduler).

I suggest introducing a validateSync feature if you want redux-logic to be able to change actions for next middlewares without having the penalty of needing to chain all the logic validations together.

I'll try to clean up a bit and commit and share my fork so you can get some ideas for your own refactor.

[jeffbski]

Thanks for responding @tehnomaag with this great summary of your changes. I'll plan to also go through these and understand the details so we might be able to bring these into the this repo as well.

[shauntrennery]

@jeffbski I'm working on an application with 465 items of logic. 465 is seemingly too many for createLogicMiddleware as I'm not getting a "Maximum call stack size exceeded" error from within RXJS. Any ideas how I could approach this problem?

[jeffbski]

@shauntrennery I'm not sure I understood you. Are you saying that you are having an issue with 465 items but don't see any error? If so explain to me what happens. Did it work fine with 464 items, but now 465 is a problem?

@askot and @erkiesken I know you were experimenting with some tweaks to the code. Did this pan out for you and if so, can you point me to what you did and suggestions on what could be done?

Some of the ideas I had originally didn't pan out once I worked through the details.

[shauntrennery]

@jeffbski correct, 465 was the magic number for me. Calling createLogicMiddleware with an array of 465 items gave me a "Maximum call stack size exceeded".

I've opted for the following approach (splitting the logic array into two) as a workaround:

const middlewares = [ routerMiddleware(history), createLogicMiddleware(core, logicDependencies), createLogicMiddleware(app, logicDependencies), noticeMiddleware ]

[jeffbski]

ok, thanks. I'll look at the changes that the @erkiesken was experimenting with to see if we can find a good way to improve this.

[askot]

@erkiesken had a patch that changed how the logics are triggered and it seemed to work around the stack size issue. In principle it was similar to triggering each logic code with setTimeout, to get a new stack for every logic that runs.

It helped the stack issue but changed the order of how logics are actually called, a lot of redux-logic tests stopped working and also existing features were broken (it affected debouncing and cancelling logics if i remember correctly). So we couldn't use it without major changes in application code.

[jeffbski]

Thanks for clarifying @askot, that helps.

I'm directing a retreat the next few days, but after that (Monday) I will be able to dive into that and see if I can figure out a solution.

[pmualaba]

@ jeffbski I guess the next obvious question is: has there been any real world failures in a redux-logic app due to this (on Mobile Safari or otherwise)? or is are you just doing due dilligence to anticipate potential problems?

Yes, same problem over here: We chose redux-logic for its scalability and capability to organize bigger and more complex applications, but now we face a showstopper just on this very point of scalability. We also have the "Max call stack exceeded" issue because we reached the maximum number of logic middlewares... We are sadly forced to introduce a separate npm package (redux-observable) to distribute the middlewares over both middlewares, until this issue is fundamentally fixed?

We would love to stick to redux-logic only though, because it is conceptually by far the best middleware for modern redux applications :-)

[shauntrennery]

@pmualaba I seperated my logic array into two; core and app. The code below has resolved the issue for us.

const middlewares = [ routerMiddleware(history), createLogicMiddleware(core, logicDependencies), createLogicMiddleware(app, logicDependencies), noticeMiddleware ]

[jeffbski]

Thanks @shauntrennery , that is a good work around.

Yes @pmualaba it sounds like when one user got to 465 logics they started having problem. The exact number may vary by JS engine.

So it seems to me that this is only affecting a few users with large number of logics. The work around by splitting into multiple middleware instances will help go beyond that. However I am going to sit down this Saturday and take a hard look to see if can find any other ideas to help this situation.

[pmualaba]

@jeffbski Thanks already for taking a look at it. In case it can not be fixed fundamentally, it could be considered to handle @shauntrennery 's workaround internally (for example auto creating of a new array every 200 logics ?), so that the splitting of logic can be abstracted away ?

[jeffbski]

thats a good idea and will keep that in mind. thanks!

On Thu, Feb 22, 2018 at 8:57 AM pmualaba [email protected] wrote:

@jeffbski https://github.com/jeffbski Thanks already for taking a look at it. In case it can not be fixed fundamentally, it could be considered to handle @shauntrennery https://github.com/shauntrennery 's workaround internally (for example auto creating of a new array every 200 logics ?), so that the splitting of logic can be abstracted away ?

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/jeffbski/redux-logic/issues/83#issuecomment-367706523, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAWOThz9nXjqNFpFdIu5Zzs9147-QTxks5tXYBRgaJpZM4QOIJT .

-- Jeff Barczewski Founder of CodeWinds http://codewinds.com/ Live and Online developer training

[pmualaba]

Hello Jeff, do we already have white smoke ;-) ?

[jeffbski]

@pmualaba not yet, but hopefully soon. I just wrapped up a big project, so I'll have time to dig into this.