null request controllerId causes application to crash

mjclawar commented 5 years ago

Description

If a controllerId is passed in as null to the requestQueue in TreeContainer.getLoadingState, the entire Dash application crashes with this exception:

react-dom@16.2.0.production.min.js?v=0.20.0&m=1552933254:162 TypeError: Cannot read property 'split' of null
    at TreeContainer.js:105
    at forEach (forEach.js:43)
    at _checkForMethod.js:22
    at f2 (_curry2.js:25)
    at recursivelyRender (TreeContainer.js:103)
    at TreeContainer.js:72
    at Array.map (<anonymous>)
    at recursivelyRender (TreeContainer.js:72)
    at TreeContainer.js:72
    at Array.map (<anonymous>)

Example data for a request in requestQueue that causes the TypeError:

{
  controllerId: null,
  status: "loading",
  uid: "be0fa2be-a851-d816-5842-e93b6264725",
  requestTime: 1552934927500
}

This line is causing the issue: https://github.com/plotly/dash-renderer/blob/8fa6d2947d271baa8b1af424e56d9de566796ba9/src/TreeContainer.js#L89

Potential solution

Change r.controllerId.split('.'); to controllerId.split('.');, which seems like the right way to handle the null value, given that it has already been redefined a few lines above. https://github.com/plotly/dash-renderer/blob/8fa6d2947d271baa8b1af424e56d9de566796ba9/src/TreeContainer.js#L86

Comments

I'm not actually sure how the controllerId is making it through as null. I can't provide a small example, because it's happening randomly in one of our larger applications, but we've definitely located it to this line, and making the change from

[loadingComponent, loadingProp] = r.controllerId.split('.');

to

[loadingComponent, loadingProp] = controllerId.split('.');

fixes the issue.

We might have some Dash components in our application that are returned with no identifiers here, but it seems like, if this is an intended exception, it should be handled by validation somewhere before making it to this stage of the dash-renderer logic. I'm not familiar enough with the request queue to immediately identify where that would occur. The seeming "randomness" of the crashing is concerning (sometimes it works, sometimes it crashes the application).

Happy to submit a PR with this change if it is actually a bug!

mjclawar commented 5 years ago

More information upon digging further:

This setup caused it:

A component using the new multiple Output updates in a single callback function
The component itself had an empty ID ("") and an empty list for children ([])
The callback updated five components at once

Not sure why it broke inconsistently still.

There should probably be some checking somewhere that ensures all IDs are not empty (we added another line to check that in our internal code), and I still think the behavior above is not necessarily desirable.

valentijnnieman commented 5 years ago

I agree that there at least should be some sort of checking for null before doing the .split – if you'd create a PR for this I'd be happy to review it! Thanks for creating this issue either way.

alexcjohnson commented 5 years ago

A 2-character bugfix - and I guess since nobody pinned down its cause we can add it without new tests so it's extra easy. But I'll wait to implement it until we get the repo merge completed.

plotly / dash