Error: Dispatch may not be called while a dispatch is in progress

[colindresj]

Ran into this exception today, and I was baffled. I'm not doing any cascading dispatches, or calling dispatch inside of an observer, etc.

After digging through a bit I discovered that an isDispatching flag is being toggled inside the reactor. Practically, that makes sense, since dispatches are supposed to be synchronous. However, a change observer's handler may be async, which opens up the potential for the flag not yet having been switched by the time another, completely separate action has been dispatched, resulting in this error.

That's exactly what was happening with my app. Larger React apps (like the one I'm working on) are especially vulnerable because setState is async and goes through batching. Of course, setState is used extensively, even by the nuclear react mixin/nuclear component.

More in general though, I think the current implementation makes it unsafe to write async code if relying on dispatching events, because you'll never truly know if that flag has been switched properly.

Potential fix could be just switching the way or moment in which the flag is toggled. Talking with a work colleague, however, we think some form of tick management might be more robust.

isDispatching logic was introduced here: fad1ef9

[jordangarcia]

Hey Jorge,

Can you elaborate a bit more on setState being async. Is that the store handler being an async function?

In the current implementation, since dispatch is always synchronous there shouldn't be a moment when a deferred change observer would want to dispatch while another dispatch is in progress.

It may be the case that you're using Nuclear in a way I didn't account for when writing this code. If so we should figure out a good solution that works in all cases.

[colindresj]

React.Component#setState is by its own nature async and goes through batching for efficient UI updates.

I'll try to put together a small fiddle to see if I can replicate the issue then post to here.

[balloob]

The reason the flag is there is because otherwise it will start dispatching
an updated state to observers before all observers have been notified of
the previous state.

On Tue, Aug 4, 2015, 00:29 JC [email protected] wrote:

React.Component#setState is by its own nature async and goes through
batching for efficient UI updates.

I'll try to put together a small fiddle to see if I can replicate the
issue then post to here.

—
Reply to this email directly or view it on GitHub
#141 (comment)
.

[mindjuice]

@colindresj Whether your observer starts an async action or not doesn't matter as far as the call to __notify() goes. Once your async action is initiated, your observer function continues running and returns, then __notify() calls the next observer, if any.

Since JavaScript runs on a single-threaded event loop, the results of your observer's async action will not be called until the next event loop at the earliest. This is long after the __notify() has notified all observers.

I think the problem lies elsewhere.

[mindjuice]

@balloob Looking at the dispatch() function, I think line 126 this.__isDispatching = false needs to go after the else if, otherwise, if the state happens not to have changed, then you won't set the flag back to false.

[balloob]

The code has been updated since that commit and the master branch has it correct: https://github.com/optimizely/nuclear-js/blob/master/src/reactor.js#L140

[mindjuice]

Oh, good. Was the old version actually released? Could @colindresj just have the old version?

[balloob]

No, the fix was included as part of v1.1 which was the first version to throw this exception.

[colindresj]

Here's a fiddle with a contrived example of what we're doing https://jsfiddle.net/acj30a5u/1/, hopefully it helps

[balloob]

I have updated your fiddle with some log statements to give a better insight in what is going on.

So it looks like when app.setState is called, it will directly call child.componentWillReceiveProps. I was under the impression from the React docs that setState would trigger a render in the next iteration of the JavaScript event loop. One workaround for now would be to wrap your setState inside the observer in a setTimeout.

(See #130 for the background of why isDispatching was implemented.)

[colindresj]

If I'm not mistaken, React will do what you're talking about if the event is synthetic (ie. managed by React). Any other kind of event that triggers setState will execute an immediate render, because React doesn't have control over it and so can't do it's batching and deferred re-application.

I initially thought that the inverse was the problem, but after looking at your updated fiddle I see what's going on. Thanks for diving into that @balloob

Manually flushing the event using a setTimeout like you're suggested would work, but it's not really scalable.

I didn't use the mixin as part of the fiddle, but the observation/setState stuff is almost exactly the same as what's part of the Nuclear React mixin. Having to manually flush means dropping the officially supported React/Nuclear integration and rolling our own. Might help us, might not, but certainly doesn't help any other users.

Even without the mixin, not being able to call setState in an observer with absolute confidence without wrapping it in a setTimeout, RAF or something like that feels like a hack.

I totally agree with the reasoning behind isDispatching, but, at least for us (and I suspect other large React apps), the current version of Nuclear is unusable.

[jordangarcia]

Apologies for not getting a chance to dive deep into this issue

One of NuclearJS' design principles is interoperability with other
libraries. If this change is causing breaking issues within normally used
React apps then that is unacceptable.

Until I have a chance to dive in I can't say for sure what the solution
will be. Perhaps making this a console.warn instead of a hard error

On Wednesday, August 5, 2015, JC [email protected] wrote:

If I'm not mistaken, React will do what you're talking about if the event
is synthetic (ie. managed by React). Anything other kind of event that
triggers setState will execute an immediate render, because React doesn't
have control over it and so can't do it's batching and deferred
re-application.

I initially thought that the inverse was the problem, but after looking at
your updated fiddle I see what's going on. Thanks for diving into that
@balloob https://github.com/balloob

Manually flushing the event using a setTimeout like you're suggested
would work, but it's not really scalable.

I didn't use the mixin as part of the fiddle, but the observation/setState
stuff is almost exactly the same as what's part of the Nuclear React mixin.
Having to manually flush means dropping the officially supported
React/Nuclear integration and rolling our own. Might help us, might not,
but certainly doesn't help any other users.

Even without the mixin, not being able to call setState in an observer
with absolute confidence without wrapping it in a setTimeout, RAF or
something like that feels like a hack.

I totally agree with the reasoning behind isDispatching, but, at least
for us (and I suspect other large React apps), the current version of
Nuclear is unusable.

—
Reply to this email directly or view it on GitHub
#141 (comment)
.

[balloob]

Looked at a few other Flux implementations, they all raise errors too:

• Flux (Facebook)
• Fluxible (Yahoo)
• Fluxxor

[bhamodi]

@jordangarcia - I was able to see this same error message in the console in our mobile editor.
Reproducing is kind of complicated. To repro I had to connect a device to the editor, launch the device in preview mode from the editor, exit preview mode from the device and then after it goes back into edit mode and the mobile editor becomes active again, the error message appears. I imagine that we get into a weird state after all these socket messages and dispatches. I can show you in person tomorrow.

[colindresj]

To be clear, I'm in favor of throwing when dispatching during a dispatch. I think that was a good decision. It's just currently causing problems for us.

[dtothefp]

@colindresj @jordangarcia I do agree this may be an issue as we have reproduced it at work using a combination of an action sent in a resolved promise and another action sent in the lifecycle method componentDidUpdate. For us wrapping the action emitted from inside the promise resolution in a setTimeout zero worked, but I agree with @colindresj that this is not a sustainable solution.

Update

I've reproduced a simplified version of our specific use case successfully in this Pen http://codepen.io/dtothefp/pen/NGpXMJ.

@jordangarcia any ideas about what is going on or potential timeline of solution?

[jordangarcia]

I'll have to dive into this issue deeper.

A potential solution is to use a console.warn instead of throwing an error until this can be properly resolved.

[balloob]

Another solution could be to queue the actions and process them once all observers have been called.

[dtothefp]

@jordangarcia if we were to temporarily switch to 'console.warn' is there actually some sort of risk of dispatcjon while another dispatch is in progress. Is this a data syncing issue?

[balloob]

Observers can be called if you do this, see #130

[colindresj]

Yeah, I think cascading dispatches is a real potential problem and something the flux spec is explicitly against, which is why I'm still pro leaving the throw in there, even if it's not working for me and others.

The real mixup is in how/when React renders. Sometime's it's after a tick, then in other scenario's it's part of the same loop. My previous comments go into why React acts this way, but, in short, it's because React doesn't have control over outside behaviors, so it can't do it's deferred rendering. The second scenario will always be the case with changes in response to async actions.

Thinking more about it though, I'm starting to feel like the code I've describe in the fiddle, is an anti-pattern, because it's easy to fall into the React render trap. Instead, it seems like the real solution is to think more about how we're designing our actions.

Understandably, changing a lot of code around to fit a better pattern (while right IMO), isn't always feasible. I did some digging around, and it seems like this works and doesn't involve a lot of refactoring to existing codebases:

const oldDispatch = reactor.dispatch.bind(reactor);
reactor.dispatch = (actionType, payload) => {
  React.addons.batchedUpdates(() => oldDispatch(actionType, payload));
};

This basically applies React's batched rendering strategy to all actions dispatch by a Nuclear reactor (regardless of synchronicity). Solves the problem in my fiddle, as well as @dtothefp's codepen (you'll get some deprecation warning because he's using React 0.14, and the package has moved).

The good news is it sounds like newer versions of React will move to always batching all updates, so this will soon be addressed by React itself.

It might be cool to ship something inside of the Nuclear react addons that does something like the code I wrote above automatically. Maybe something like:

import { performBatchedUpdates }  from 'nuclear-js-react-addons';

reactor = performBatchedUpdates(reactor);
// This reactor now does all of its dispatches inside of a batchedUpdates callback.

Another option might be to allow users to wrap Reactor#dispatch with custom logic (which could be batchUpdating or anything else). Here's an off-the-cuff API:

reactor.wrapDispatches((dispatchFn, actionType, payload) => {
  // do something in here then call dispatchFn(actionType, payload)
  // so you could do: React.addons.batchedUpdates(() => dispatchFn(actionType, payload));
  // or something else you might need
});

[colindresj]

@jordangarcia any thoughts on the above?

[fabiosussetto]

Just experienced the same problem with this snippet:

componentDidMount () {
    this.fetchEvent()
  }

fetchEvent () {
    const eventId = parseInt(this.props.params.eventId, 10)
    eventActions.fetch(eventId)
      .then(() => {
        eventActions.setCurrentEvent(eventId)
      })
      .done()
 }

This is causing the dispatch in progress error as there's another component that is doing almost the same in the meanwhile. Moving the eventActions.setCurrentEvent(eventId) call outside of the then() method fixes the issue, but I can't really do that as I need to update the UI with the new loaded data only once the new data has arrived from the API (to avoid flickering in the layout).

If anyone has an idea on how to achieve the same without using promises, I'm all ears.

It looks like the approach suggested by @colindresj above is also suggested by Fluxxor: http://fluxxor.com/guides/react.html#batched-updates

So @colindresj approach solved my problem, for reference I had to slightly change it to use it with React 0.14:

import ReactDom from 'react-dom'

const oldDispatch = reactor.dispatch.bind(reactor);
reactor.dispatch = (actionType, payload) => {
  ReactDom.unstable_batchedUpdates(() => oldDispatch(actionType, payload))
}