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

perf: cancel dashboard page requests #19029

Merged
merged 6 commits into from
Jul 23, 2020
Merged

perf: cancel dashboard page requests #19029

merged 6 commits into from
Jul 23, 2020

Conversation

121watts
Copy link
Contributor

@121watts 121watts commented Jul 22, 2020

Closes #18640

The Problem

A bunch of slow and unresolved requests to /query would continue to fetch despite the user navigating away from the page. Thus making the rest of their experience slow and 💩

The Solution

When the the Dashboard Page un-mounts cancel any requests to /query that are in flight.

Kapture 2020-07-22 at 16 39 05

@121watts 121watts requested review from a team and TCL735 and removed request for a team July 23, 2020 00:10
@TCL735
Copy link
Contributor

TCL735 commented Jul 23, 2020

🔥

Copy link
Contributor

@hoorayimhelping hoorayimhelping left a comment

Choose a reason for hiding this comment

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

🤞


while (cached.length) {
curr = cached.pop()
curr.resolve.apply(curr, args) // eslint-disable-line prefer-spread
Copy link
Contributor

Choose a reason for hiding this comment

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

curious about the apply here rather than just calling curr.resolve(...args) since it's using curr for context

@@ -363,6 +370,10 @@ export const getDashboard = (dashboardID: string) => async (
dispatch(creators.setDashboard(dashboardID, RemoteDataState.Done, normDash))
dispatch(updateTimeRangeFromQueryParams(dashboardID))
} catch (error) {
if (error.name === 'AbortError') {
Copy link
Contributor

Choose a reason for hiding this comment

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

we allso check for 'CancellationError' in some handlers. not sure if we need that here, but wanted to bring it up

Copy link
Contributor

@asalem1 asalem1 left a comment

Choose a reason for hiding this comment

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

Great work. I am curious why we didn’t go down the route of triggering a lifecycle event on the TimeSeries.tsx component since we’d have direct access to the pendingResults and can simply cancel the cell queries from there? Unless that’s what’s currently happening and the event isn’t being handled correctly?

More specifically to the point. The TimeSeries cell has a reference to the runQuery result that’s generated (which in and of itself generates an abortController). We can call cancel using the pending results on the unmount:

private pendingResults: Array<CancelBox<RunQueryResult>> = []

and you'll see how it's being used here:

// Cancel any existing queries
this.pendingResults.forEach(({cancel}) => cancel())

for runQuery. this will allow only one query to be run at a time, and
pass its results to any calls made to it while waiting for the api.
usage:
const mutex = RunQueryPromiseMutex()
Copy link
Contributor

Choose a reason for hiding this comment

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

Where have I seen this before?

@121watts 121watts merged commit d3c86e8 into master Jul 23, 2020
@121watts 121watts deleted the fix/cancel-pending branch July 23, 2020 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Navigating away from a dashboard should cancel all pending queries for that dashboard
5 participants