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

feat: cancel queries on navigation-ish #13414

Merged
merged 19 commits into from
Dec 29, 2022
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 6 additions & 1 deletion frontend/src/scenes/dashboard/Dashboard.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,15 @@ function DashboardScene(): JSX.Element {
dashboardMode,
receivedErrorsFromAPI,
} = useValues(dashboardLogic)
const { setDashboardMode, setDates, reportDashboardViewed, setProperties } = useActions(dashboardLogic)
const { setDashboardMode, setDates, reportDashboardViewed, setProperties, abortAnyRunningQuery } =
useActions(dashboardLogic)

useEffect(() => {
reportDashboardViewed()
return () => {
Copy link
Contributor

@macobo macobo Dec 20, 2022

Choose a reason for hiding this comment

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

Suggestion: create a useQueryCancellation(fn) wrapping useEffect.

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem with wrapping (or at least the problem I've not figured out the solution to) is the insight and dashboard cancellation cases are still really different

// request cancellation of any running queries when this component is no longer in the dom
abortAnyRunningQuery()
}
}, [])

useKeyboardHotkeys(
Expand Down
21 changes: 10 additions & 11 deletions frontend/src/scenes/dashboard/dashboardLogic.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -187,9 +187,10 @@ export const dashboardLogic = kea<dashboardLogicType>([
loadingDashboardItemsStarted: (action: string, dashboardQueryId: string) => ({ action, dashboardQueryId }),
setInitialLoadResponseBytes: (responseBytes: number) => ({ responseBytes }),
abortQuery: (payload: { dashboardQueryId: string; queryId: string; queryStartTime: number }) => payload,
abortAnyRunningQuery: true,
}),

loaders(({ actions, props, values, cache }) => ({
loaders(({ actions, props, values }) => ({
// TODO this is a terrible name... it is "dashboard" but there's a "dashboard" reducer ¯\_(ツ)_/¯
allItems: [
null as DashboardType | null,
Expand Down Expand Up @@ -225,10 +226,7 @@ export const dashboardLogic = kea<dashboardLogicType>([
return values.allItems
}

if (cache.abortController) {
cache.abortController.abort()
}
cache.abortController = null
actions.abortAnyRunningQuery()

try {
return await api.update(`api/projects/${values.currentTeamId}/dashboards/${props.id}`, {
Expand Down Expand Up @@ -778,7 +776,6 @@ export const dashboardLogic = kea<dashboardLogicType>([
}
},
beforeUnmount: () => {
cache.abortController?.abort()
if (cache.autoRefreshInterval) {
window.clearInterval(cache.autoRefreshInterval)
cache.autoRefreshInterval = null
Expand Down Expand Up @@ -926,11 +923,8 @@ export const dashboardLogic = kea<dashboardLogicType>([
true
)

// If a query is in progress, kill that query
// we will use one abort controller for all insight queries for this dashboard
if (cache.abortController) {
cache.abortController.abort()
}
actions.abortAnyRunningQuery()
cache.abortController = new AbortController()
const methodOptions: ApiMethodOptions = {
signal: cache.abortController.signal,
Expand Down Expand Up @@ -997,7 +991,6 @@ export const dashboardLogic = kea<dashboardLogicType>([
refreshesFinished += 1
if (refreshesFinished === insights.length) {
breakpoint()
cache.abortController = null

const payload: TimeToSeeDataPayload = {
type: 'dashboard_load',
Expand Down Expand Up @@ -1162,6 +1155,12 @@ export const dashboardLogic = kea<dashboardLogicType>([
actions.setShouldReportOnAPILoad(true)
}
},
abortAnyRunningQuery: () => {
if (cache.abortController) {
cache.abortController.abort()
cache.abortController = null
}
},
abortQuery: async ({ dashboardQueryId, queryId, queryStartTime }) => {
const { currentTeamId } = values
if (values.featureFlags[FEATURE_FLAGS.CANCEL_RUNNING_QUERIES]) {
Expand Down
23 changes: 22 additions & 1 deletion frontend/src/scenes/insights/Insight.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,17 @@ export function Insight({ insightId }: { insightId: InsightShortId | 'new' }): J
tagLoading,
insightSaving,
exporterResourceParams,
showErrorMessage,
} = useValues(logic)
useMountedLogic(insightCommandLogic(insightProps))
const { saveInsight, setInsightMetadata, saveAs, reportInsightViewedForRecentInsights } = useActions(logic)
const {
saveInsight,
setInsightMetadata,
saveAs,
reportInsightViewedForRecentInsights,
abortAnyRunningQuery,
loadResults,
} = useActions(logic)
const { duplicateInsight, loadInsights } = useActions(savedInsightsLogic)

const { hasAvailableFeature } = useValues(userLogic)
Expand All @@ -74,6 +82,19 @@ export function Insight({ insightId }: { insightId: InsightShortId | 'new' }): J
reportInsightViewedForRecentInsights()
}, [insightId])

useEffect(() => {
if (showErrorMessage) {
macobo marked this conversation as resolved.
Show resolved Hide resolved
// if mounting and logic is already in error state, then
// this insight was probably cancelled on navigation and
// it is safe to reload
loadResults()
}
return () => {
// request cancellation of any running queries when this component is no longer in the dom
abortAnyRunningQuery()
}
}, [])

const usingEditorPanels = featureFlags[FEATURE_FLAGS.INSIGHT_EDITOR_PANELS]

// Show the skeleton if loading an insight for which we only know the id
Expand Down
25 changes: 13 additions & 12 deletions frontend/src/scenes/insights/insightLogic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,7 @@ export const insightLogic = kea<insightLogicType>([
toggleVisibility: (index: number) => ({ index }),
setHiddenById: (entry: Record<string, boolean | undefined>) => ({ entry }),
highlightSeries: (seriesIndex: number | null) => ({ seriesIndex }),
abortAnyRunningQuery: true,
}),
loaders(({ actions, cache, values, props }) => ({
insight: [
Expand Down Expand Up @@ -295,11 +296,11 @@ export const insightLogic = kea<insightLogicType>([
// fetch this now, as it might be different when we report below
const scene = sceneLogic.isMounted() ? sceneLogic.values.scene : null

// If a query is in progress, kill that query
if (cache.abortController) {
cache.abortController.abort()
}
actions.abortAnyRunningQuery()
cache.abortController = new AbortController()
const methodOptions: ApiMethodOptions = {
signal: cache.abortController.signal,
}

const { filters } = values

Expand All @@ -316,9 +317,6 @@ export const insightLogic = kea<insightLogicType>([
let apiUrl: string = ''
const { currentTeamId } = values

const methodOptions: ApiMethodOptions = {
signal: cache.abortController.signal,
}
if (!currentTeamId) {
throw new Error("Can't load insight before current project is determined.")
}
Expand Down Expand Up @@ -355,7 +353,6 @@ export const insightLogic = kea<insightLogicType>([
})
}
breakpoint()
cache.abortController = null
actions.endQuery({
queryId,
view: insight,
Expand All @@ -380,7 +377,6 @@ export const insightLogic = kea<insightLogicType>([
}

breakpoint()
cache.abortController = null
actions.endQuery({
queryId,
view: (values.filters.insight as InsightType) || InsightType.TRENDS,
Expand Down Expand Up @@ -749,7 +745,7 @@ export const insightLogic = kea<insightLogicType>([
},
],
}),
listeners(({ actions, selectors, values }) => ({
listeners(({ actions, selectors, values, cache }) => ({
setFiltersMerge: ({ filters }) => {
actions.setFilters({ ...values.filters, ...filters })
},
Expand Down Expand Up @@ -862,6 +858,12 @@ export const insightLogic = kea<insightLogicType>([
)
actions.setIsLoading(true)
},
abortAnyRunningQuery: () => {
if (cache.abortController) {
cache.abortController.abort()
cache.abortController = null
}
},
abortQuery: async ({ queryId }) => {
const { currentTeamId } = values

Expand Down Expand Up @@ -1055,7 +1057,7 @@ export const insightLogic = kea<insightLogicType>([
}
},
})),
events(({ props, cache, values, actions }) => ({
events(({ props, values, actions }) => ({
afterMount: () => {
if (!props.cachedInsight || !props.cachedInsight?.result || !!props.cachedInsight?.filters) {
if (
Expand Down Expand Up @@ -1088,7 +1090,6 @@ export const insightLogic = kea<insightLogicType>([
}
},
beforeUnmount: () => {
cache.abortController?.abort()
if (values.timeout) {
clearTimeout(values.timeout)
}
Expand Down