Skip to content

Issue 1010 - Non-rendered async components using asyncDecorator lock renderer callbacks #1027

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

Merged
merged 14 commits into from
Nov 26, 2019
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ clientside JavaScript callbacks via inline strings.

### Fixed
- [#1018](https://github.com/plotly/dash/pull/1006) Fix the `dash.testing` **stop** API with process application runner in Python2. Use `kill()` instead of `communicate()` to avoid hanging.
- [#1027](https://github.com/plotly/dash/pull/1027) Fix bug with renderer callback lock never resolving with non-rendered async component using the asyncDecorator

## [1.6.1] - 2019-11-14
### Fixed
Expand Down
36 changes: 31 additions & 5 deletions dash-renderer/src/actions/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
slice,
sort,
type,
uniq,
view,
} from 'ramda';
import {createAction} from 'redux-actions';
Expand Down Expand Up @@ -221,11 +222,12 @@ export function notifyObservers(payload) {
return async function(dispatch, getState) {
const {id, props, excludedOutputs} = payload;

const {graphs, isAppReady, requestQueue} = getState();

if (isAppReady !== true) {
await isAppReady;
}
const {
dependenciesRequest,
graphs,
isAppReady,
requestQueue,
} = getState();

const {InputGraph} = graphs;
/*
Expand Down Expand Up @@ -365,6 +367,30 @@ export function notifyObservers(payload) {
}
});

/**
* Determine the id of all components used as input or state in the callbacks
* triggered by the props change.
*
* Wait for all components associated to these ids to be ready before initiating
* the callbacks.
*/
const deps = queuedObservers.map(output =>
dependenciesRequest.content.find(
dependency => dependency.output === output
)
);

const ids = uniq(
flatten(
deps.map(dep => [
dep.inputs.map(input => input.id),
dep.state.map(state => state.id),
])
)
);

await isAppReady(ids);

/*
* record the set of output IDs that will eventually need to be
* updated in a queue. not all of these requests will be fired in this
Expand Down
82 changes: 24 additions & 58 deletions dash-renderer/src/actions/setAppReadyState.js
Original file line number Diff line number Diff line change
@@ -1,54 +1,33 @@
import {filter} from 'ramda';
import {createAction} from 'redux-actions';
import { path } from 'ramda';
import { createAction } from 'redux-actions';

import isSimpleComponent from '../isSimpleComponent';
import Registry from './../registry';
import {getAction} from './constants';
import {isReady} from '@plotly/dash-component-plugins';
import { getAction } from './constants';
import { isReady } from '@plotly/dash-component-plugins';

const isAppReady = layout => {
const queue = [layout];

const res = {};
const isAppReady = (layout, paths) => targets => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as we get the order of operations right (ie always updating isAppReady after any update to layout or paths) this should be fine, but it would be more robust and simpler - without any cost anymore - to not put this in the state at all.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ie to just make it a function (layout, paths, targets) => {...}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exactly the problem making the tests fail, the store evals IS_APP_READY before COMPUTE_PATH and the first calls ends up not blocking

if (!layout || !paths || !Array.isArray(targets)) {
return true;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a use case for this? Feels like an error rather than a silent success.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Relates to the comment below. Refactoring to take isAppReady outside of the state, timing issues will be moot.

}

/* Would be much simpler if the Registry was aware of what it contained... */
while (queue.length) {
const elementLayout = queue.shift();
if (!elementLayout) {
continue;
const promises = [];
targets.forEach(id => {
const pathOfId = paths[id];
if (!pathOfId) {
return;
}

const children = elementLayout.props && elementLayout.props.children;
const namespace = elementLayout.namespace;
const type = elementLayout.type;

res[namespace] = res[namespace] || {};
res[namespace][type] = type;

if (children) {
const filteredChildren = filter(
child => !isSimpleComponent(child),
Array.isArray(children) ? children : [children]
);

queue.push(...filteredChildren);
const target = path(pathOfId, layout);
if (!target) {
return;
}
}

const promises = [];
Object.entries(res).forEach(([namespace, item]) => {
Object.entries(item).forEach(([type]) => {
const component = Registry.resolve({
namespace,
type,
});

const ready = isReady(component);
const component = Registry.resolve(target);
const ready = isReady(component);

if (ready && typeof ready.then === 'function') {
promises.push(ready);
}
});
if (ready && typeof ready.then === 'function') {
promises.push(ready);
}
});

return promises.length ? Promise.all(promises) : true;
Expand All @@ -57,21 +36,8 @@ const isAppReady = layout => {
const setAction = createAction(getAction('SET_APP_READY'));

export default () => async (dispatch, getState) => {
const ready = isAppReady(getState().layout);
const { layout, paths } = getState();
const ready = isAppReady(layout, paths);

if (ready === true) {
/* All async is ready */
dispatch(setAction(true));
} else {
/* Waiting on async */
dispatch(setAction(ready));
await ready;
/**
* All known async is ready.
*
* Callbacks were blocked while waiting, we can safely
* assume that no update to layout happened to invalidate.
*/
dispatch(setAction(true));
}
dispatch(setAction(ready));
};
23 changes: 15 additions & 8 deletions dash-renderer/tests/notifyObservers.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@ const WAIT = 1000;
describe('notifyObservers', () => {
const thunk = notifyObservers({
id: 'id',
props: {},
props: {
'prop1': true
},
undefined
});

Expand All @@ -16,13 +18,16 @@ describe('notifyObservers', () => {
() => ({
graphs: {
InputGraph: {
hasNode: () => false,
dependenciesOf: () => [],
hasNode: () => true,
dependenciesOf: () => [
'id.prop2'
],
dependantsOf: () => [],
overallOrder: () => 0
}
},
isAppReady: true,
isAppReady: () => Promise.resolve(true),
paths: {},
requestQueue: []
})
).then(() => { done = true; });
Expand All @@ -33,23 +38,25 @@ describe('notifyObservers', () => {

it('waits on app to be ready', async () => {
let resolve;
const isAppReady = new Promise(r => {
const isAppReady = () => new Promise(r => {
resolve = r;
});

let done = false;
thunk(
() => { },
() => ({
graphs: {
InputGraph: {
hasNode: () => false,
dependenciesOf: () => [],
hasNode: () => true,
dependenciesOf: () => [
'id.prop2'
],
dependantsOf: () => [],
overallOrder: () => 0
}
},
isAppReady,
paths: {},
requestQueue: []
})
).then(() => { done = true; });
Expand Down
36 changes: 35 additions & 1 deletion tests/integration/callbacks/test_basic_callback.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@

import dash_core_components as dcc
import dash_html_components as html
import dash_table
import dash
from dash.dependencies import Input, Output
from dash.dependencies import Input, Output, State
from dash.exceptions import PreventUpdate


def test_cbsc001_simple_callback(dash_duo):
Expand Down Expand Up @@ -140,3 +142,35 @@ def update_input(value):

dash_duo.percy_snapshot(name="callback-generating-function-2")
assert dash_duo.get_logs() == [], "console is clean"


def test_cbsc003_callback_with_unloaded_async_component(dash_duo):
app = dash.Dash()
app.layout = html.Div(
children=[
dcc.Tabs(
children=[
dcc.Tab(
children=[
html.Button(id="btn", children="Update Input"),
html.Div(id="output", children=["Hello"]),
]
),
dcc.Tab(children=dash_table.DataTable(id="other-table")),
]
)
]
)

@app.callback(Output("output", "children"), [Input("btn", "n_clicks")])
def update_graph(n_clicks):
if n_clicks is None:
raise PreventUpdate

return "Bye"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before the changes in this PR, this callback is never triggered because of the blocking dash-table (uses the asyncDecorator) in the other tab. Now, since the callback only involves btn and output, the table is non-blocking.


dash_duo.start_server(app)

dash_duo.find_element('#btn').click()
assert dash_duo.find_element('#output').text == "Bye"
assert dash_duo.get_logs() == []