onBrushEnd should be called after setState is completed

[angorayc]

Describe the bug
If onBrushEnd is given, it should call it after setState is completed, otherwise it occurs warnings about setting state when unmount.

For example, this is the function I am passing to onBrushEnd:

(min: number, max: number) => {
  setTimeout(() => {
    setAbsoluteRangeDatePicker({ id: 'global', from: min, to: max });
  }, 500);
}

The reason why I'm waiting for 500ms here is to make sure setState is completed.
https://github.com/elastic/elastic-charts/blob/master/src/components/react_canvas/reactive_chart.tsx#L305

Expected behavior
onBrushEnd should be called after setState is completed

Screenshots
If I remove setTimeout:

Errors in browser console

backend.js:1 Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in the componentWillUnmount method.
    in ReactiveChart
    in inject-ReactiveChart-with-chartStore
    in div
    in ChartContainer
    in inject-ChartContainer-with-chartStore
    in div
    in div
    in Provider
    in Chart
    in div
    in Unknown
    in Unknown (created by AutoSizer)
    in div (created by WrappedByAutoSizer)
    in WrappedByAutoSizer (created by AutoSizer)
    in AutoSizer
    in Unknown
    in div (created by EuiFlexItem)
    in EuiFlexItem (created by FlexItem)
    in FlexItem
    in div (created by EuiFlexGroup)
    in EuiFlexGroup
    in div (created by EuiPanel)
    in EuiPanel
    in div (created by EuiFlexItem)
    in EuiFlexItem (created by FlexItem)
    in FlexItem
    in Unknown
    in div (created by EuiFlexGroup)
    in EuiFlexGroup

Kibana Cross Issues
Add any Kibana related issues here.

Checklist

• every related Kibana issue is listed under Kibana Cross Issues list
• kibana cross issue tag is associated to the issue if any kibana cross issue is present

[FrankHassanabad]

If onBrush callback is invoked as the second argument of setState:
https://reactjs.org/docs/react-component.html#setstate

We should be good I think to remove the setTimeout

The second parameter to setState() is an
optional callback function that will be executed once
setState is completed and the component is re-rendered.

In the stack trace above @angorayc , there is an inner stack trace which points to the onBrushEnd as the culprit:

r | @ | backend.js:1
-- | -- | --
  | warningWithoutStack | @ | react-dom.development.js:506
  | warnAboutUpdateOnUnmounted | @ | react-dom.development.js:18269
  | scheduleWork | @ | react-dom.development.js:19679
  | enqueueSetState | @ | react-dom.development.js:11141
  | Component.setState | @ | react.development.js:335
  | Chart._this.onEndBrushing | @ | reactive_chart.js:250

[nickofthyme]

Sorry for the delay, I'll look into this today!

[nickofthyme]

@angorayc I think I see the issue but I am not able to reproduce the error locally. Can you provide your Chart implementation snippet? Just so I can make sure I address your error.

Just looking at the code in question I see that there is a potential for a race condition to occur if this window mouseup event listener

elastic-charts/src/components/react_canvas/reactive_chart.tsx

Line 302 in 94d5a51

window.addEventListener('mouseup', this.onEndBrushing);

is invoked before the state is updated here

elastic-charts/src/components/react_canvas/reactive_chart.tsx

Lines 305 to 309 in 94d5a51

	this.setState(() => ({
	brushing: true,
	brushStart: point,
	brushEnd: point,
	}));

It seems like this scenario is unlikely for a user to be able to click AND drag before the react state is updated. But if that is the case we can handle that by only calling the listener when the state is updated.

As soon as you send me a snippet I can understand the root cause of your error.

[FrankHassanabad]

Hi @nickofthyme,

The lines of code are for our hosts page:
https://github.com/elastic/kibana/blob/master/x-pack/legacy/plugins/siem/public/pages/hosts/hosts.tsx#L105

And here for our network page:
https://github.com/elastic/kibana/blob/master/x-pack/legacy/plugins/siem/public/pages/network/network.tsx#L125

If you remove that setTimeout in either page and load the page from one of their URL's while running an audit beat with our plugin enabled to get some data:

${root}/app/siem#/hosts/

${root}/app/siem#/network

you will see the error. What happens is during a onBrushEnd we change the URL and timestamp and that can cause a remount via unmount and mount at which point the previously mounted component is trying to do cleanup via the setState and then react throws up the warning.

Here is where we trigger a redux change which eventually can lead to a unmount and mount again as it changes the time range and url which triggers react-router:

https://github.com/elastic/kibana/blob/master/x-pack/legacy/plugins/siem/public/components/stat_items/index.tsx#L274

[nickofthyme]

@FrankHassanabad sorry I was looking at the wrong setState call when I made that previous comment. Not this one 👇

elastic-charts/src/components/react_canvas/reactive_chart.tsx

Lines 323 to 328 in eb482be

	this.props.chartStore!.onBrushEnd(brushStart, brushEnd);
	this.setState(() => ({
	brushing: false,
	brushStart: { x: 0, y: 0 },
	brushEnd: { x: 0, y: 0 },
	}));

#376 should fix it. I tested it locally in SIEM and worked without error.

Also thanks for showing me how to ingest data with auditbeat!! 👍

[markov00]

🎉 This issue has been resolved in version 12.0.2 🎉

The release is available on:

• GitHub release
• npm package (@latest dist-tag)

Your semantic-release bot 📦🚀