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.

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

Risk of cascading actions #273

Closed
attekei opened this issue Mar 1, 2015 · 6 comments
Closed

Risk of cascading actions #273

attekei opened this issue Mar 1, 2015 · 6 comments

Comments

@attekei
Copy link

attekei commented Mar 1, 2015

Hi!

In contrast to Dispatcher of Flux, Reflux doesn't prevent actions from cascading in any way. Two situations when this may occur:

  1. Store listens to an action. In the listener, store triggers another action.
  2. Component listens to a store. In the listener, component triggers another action.

For experienced developers this shouldn't be a problem — they know that stores and components should not trigger actions as a reaction to other action. But if an inexperienced developer jumps into Reflux project, they may easily trigger an action in the wrong place and as a consequence, create an event chain (or an endless loop) that makes code more difficult to reason about.

This is problematic to fix because the reactivity (aka deferring) of Actions is one of the Reflux's fundamental design decisions. Any thoughts on this?

@spoike
Copy link
Member

spoike commented Mar 8, 2015

Three thoughts:

  • We do have a recursive check between stores and it is a simple one during bootstrap of your web app that will throw an error if you've managed to chain stores in recursion.
  • During runtime you'll notice quickly if you've managed to do a recursive data flow since the application will most likely grind to a halt or be unbearably sluggish. By then even junior developers need to sit down and think it through a little bit.
  • One time I managed to find a runtime recursion where there was a setTimeout in the recursion. None of the devs noticed this until the application was running a longer while and started to get sluggish. This is quite easy to find in a profiler (such as the one in Chrome Developer Tools) because activity will be shown even though there is no user input happening. It was an easy fix, since profiles usually show the stack trace, and it was due to a missing check in a component's didComponentUpdate that invoked an action.

In summary I don't personally think this is a big deal, since all you have to do to fix is to ponder on it some more and straighten those data flows out.

@spoike spoike added the question label Mar 8, 2015
@valscion
Copy link

valscion commented Mar 8, 2015

During runtime you'll notice quickly if you've managed to do a recursive data flow since the application will most likely grind to a halt or be unbearably sluggish. By then even junior developers need to sit down and think it through a little bit.

Perhaps the original concern was more pointed towards maintainability rather than performance (me and @Attrck thought about this issue together). Having an action trigger any sort of cascading effects in terms of creating more actions might make the code itself more difficult to reason about, and that is one thing the Flux dispatcher tackles with its invariants, if I've understood it correctly. Application using reflux might never enter an endless loop but just create a weird chain of events.

If I've understood correctly, you say that recursive data flows should be prevented by the developer. What if the accidental recursive flow is triggered only in some rare event in production? As reflux is not able to raise any kinds of errors, there won't be a way to catch those errors to some exception monitoring service (such as Honeybadger). This feels just like some boobytrap ready to explode once applications leveraging Reflux get larger.

I understand that I might be a bit paranoid with my claims here. The reason why we brought up this subject was merely to hear whether this drawback was a concious decision. Not being able to catch/stop recursive data flows with errors seems to be one big functional difference between Flux and Reflux.

@spoike
Copy link
Member

spoike commented Mar 8, 2015

What if the accidental recursive flow is triggered only in some rare event in production?

That is a risk, yes.

So I've been dog fooding Reflux since it's inception, and we're almost at one year mark. I even have a junior developer working with it who never really had any problems learning it (and even started using e.g. Listenables feature before I had a chance to use it myself). :-D

From a maintenance standpoint we never really had any boobytraps that I can think of is due to any of the libraries we're using.

Not being able to catch/stop recursive data flows with errors seems to be one big functional difference between Flux and Reflux.

As I previously stated, stores and joins has a recursive check that throws errors. Components listening to actions and invoking the same is something I don't see enforcing in Reflux at the moment mostly due to the default implementation be an asynchronous invocation. That is unless someone manages to write a PR that can do the check. ;-)

@valscion
Copy link

valscion commented Mar 9, 2015

Thanks for the clarifications :) glad to hear that Reflux is working great for you!

@mwolson
Copy link

mwolson commented Jun 6, 2015

The default implementation doing asynchronous invocation caused us to have sporadically failing Selenium tests. We use Reflux.createActions to create all of our actions. Noticed that running actions in a tight loop was causing a race condition:

2 UI components rely on the same store; first component is interacted with, fetches from store, calls action with value+1 from that store, second component has an action listener that writes a new value into that store. If you perform that initial interaction in a for-loop that runs three times, you'll see:

read 1, read 1, read 1, [loop ends] write 2, write 2, write 2

If this were synchronous by default, we'd get:

read 1, write 2, read 2, write 3, read 3, write 4 [loop ends]

While it's possible to iterate through each action after the Reflux.createActions call and set action.sync = true, better for us would have been to instruct new users to do a synchronous setup by default, and give them an easy way to do that in batch.

@LongLiveCHIEF
Copy link

I think this also relates to #173 . I think actions in general need a bit more thought... (or perhaps a bit less thought). Closing this in favor of #173 , added to roadmap for v0.3.0 candidate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants