Skip to content
This repository has been archived by the owner on Mar 31, 2024. It is now read-only.

Commit

Permalink
Cancel visualize fetches when navigating away or re-fetching (elastic…
Browse files Browse the repository at this point in the history
…#42035)

* add AbortSignal to interpreter

* adding AbortSignal to visualize_loader

* adding AbortSignal to embeddables and dashboard

* passing AbortSignal to courier request handler

* Remove abort signal from dashboard and move to handler, and abort fetches when necessary

* Remove the rest of the references to abort signal in dashboard

* Revert changes to dashboard_app

* Remove code related to canceling visualize requests and only keep stuff for canceling interpreter

* Use createError

* Cancel in-progress fetches when a new one is requested or when leaving the page

* Update with cancel methods and make visualize data loader work

* Remove unnecessary call to

* Fix tests

* Remove cancel from data loaders

* Fix search source to always either resolve or reject in fetch()

* Fix naming

* Update search request to reject on abort, and don't render errors in the UI

* Update tests

* Add cancellation to input control

* Cancel histogram fetches

* Update esaggs typings

* Add cancellation for search embeddables

* Abort post-flight requests
  • Loading branch information
lukasolson authored Aug 28, 2019
1 parent b0df7a8 commit 9fecf8d
Show file tree
Hide file tree
Showing 19 changed files with 157 additions and 48 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,10 @@ export class Control {
throw new Error('fetch method not defined, subclass are required to implement');
}

destroy() {
throw new Error('destroy method not defined, subclass are required to implement');
}

format = (value) => {
const field = this.filterManager.getField();
if (field) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,12 @@ const termsAgg = ({ field, size, direction, query }) => {
class ListControl extends Control {

fetch = async (query) => {
// Abort any in-progress fetch
if (this.abortController) {
this.abortController.abort();
}
this.abortController = new AbortController();

const indexPattern = this.filterManager.getIndexPattern();
if (!indexPattern) {
this.disable(noIndexPatternMsg(this.controlParams.indexPattern));
Expand Down Expand Up @@ -114,13 +120,18 @@ class ListControl extends Control {
indexPattern,
aggs,
this.useTimeFilter,
ancestorFilters);
ancestorFilters
);
this.abortController.signal.addEventListener('abort', () => searchSource.cancelQueued());

this.lastQuery = query;
let resp;
try {
resp = await searchSource.fetch();
} catch(error) {
// If the fetch was aborted then no need to surface this error in the UI
if (error.name === 'AbortError') return;

this.disable(i18n.translate('inputControl.listControl.unableToFetchTooltip', {
defaultMessage: 'Unable to fetch terms, error: {errorMessage}',
values: { errorMessage: error.message }
Expand Down Expand Up @@ -148,6 +159,10 @@ class ListControl extends Control {
this.disabledReason = '';
}

destroy() {
if (this.abortController) this.abortController.abort();
}

hasValue() {
return typeof this.value !== 'undefined' && this.value.length > 0;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,12 @@ const minMaxAgg = (field) => {
class RangeControl extends Control {

async fetch() {
// Abort any in-progress fetch
if (this.abortController) {
this.abortController.abort();
}
this.abortController = new AbortController();

const indexPattern = this.filterManager.getIndexPattern();
if (!indexPattern) {
this.disable(noIndexPatternMsg(this.controlParams.indexPattern));
Expand All @@ -60,11 +66,15 @@ class RangeControl extends Control {

const aggs = minMaxAgg(indexPattern.fields.byName[fieldName]);
const searchSource = createSearchSource(this.kbnApi, null, indexPattern, aggs, this.useTimeFilter);
this.abortController.signal.addEventListener('abort', () => searchSource.cancelQueued());

let resp;
try {
resp = await searchSource.fetch();
} catch(error) {
// If the fetch was aborted then no need to surface this error in the UI
if (error.name === 'AbortError') return;

this.disable(i18n.translate('inputControl.rangeControl.unableToFetchTooltip', {
defaultMessage: 'Unable to fetch range min and max, error: {errorMessage}',
values: { errorMessage: error.message }
Expand All @@ -84,6 +94,10 @@ class RangeControl extends Control {
this.max = max;
this.enable = true;
}

destroy() {
if (this.abortController) this.abortController.abort();
}
}

export async function rangeControlFactory(controlParams, kbnApi, useTimeFilter) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,14 +42,13 @@ class VisController {
this.controls = [];
this.controls = await this.initControls();
this.drawVis();
return;
}
return;
}

destroy() {
this.updateSubsciption.unsubscribe();
unmountComponentAtNode(this.el);
this.controls.forEach(control => control.destroy());
}

drawVis = () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ export const esaggs = (): ExpressionFunction<typeof name, Context, Arguments, Re
help: '',
},
},
async fn(context, args, handlers) {
async fn(context, args, { inspectorAdapters, abortSignal }) {
const $injector = await chrome.dangerouslyGetActiveInjector();
const Private: Function = $injector.get('Private');
const indexPatterns = Private(IndexPatternsProvider);
Expand All @@ -112,14 +112,15 @@ export const esaggs = (): ExpressionFunction<typeof name, Context, Arguments, Re
forceFetch: true,
metricsAtAllLevels: args.metricsAtAllLevels,
partialRows: args.partialRows,
inspectorAdapters: handlers.inspectorAdapters,
inspectorAdapters,
queryFilter,
abortSignal: (abortSignal as unknown) as AbortSignal,
});

const table: KibanaDatatable = {
type: 'kibana_datatable',
rows: response.rows,
columns: response.columns.map(column => {
columns: response.columns.map((column: any) => {
const cleanedColumn: KibanaDatatableColumn = {
id: column.id,
name: column.name,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -761,6 +761,9 @@ function discoverController(
})
.then(onResults)
.catch((error) => {
// If the request was aborted then no need to surface this error in the UI
if (error instanceof Error && error.name === 'AbortError') return;

const fetchError = getPainlessError(error);

if (fetchError) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ export class SearchEmbeddable extends Embeddable<SearchInput, SearchOutput>
if (this.autoRefreshFetchSubscription) {
this.autoRefreshFetchSubscription.unsubscribe();
}
this.savedSearch.searchSource.cancelQueued();
}

private initializeSearchScope() {
Expand Down Expand Up @@ -270,6 +271,10 @@ export class SearchEmbeddable extends Embeddable<SearchInput, SearchOutput>
if (!this.searchScope) return;

const { searchSource } = this.savedSearch;

// Abort any in-progress requests
searchSource.cancelQueued();

searchSource.setField('size', config.get('discover:sampleSize'));
searchSource.setField('sort', getSort(this.searchScope.sort, this.searchScope.indexPattern));

Expand Down Expand Up @@ -304,6 +309,9 @@ export class SearchEmbeddable extends Embeddable<SearchInput, SearchOutput>
this.searchScope!.totalHitCount = resp.hits.total;
});
} catch (error) {
// If the fetch was aborted, no need to surface this in the UI
if (error.name === 'AbortError') return;

toastNotifications.addError(error, {
title: i18n.translate('kbn.embeddable.errorTitle', {
defaultMessage: 'Error fetching data',
Expand Down
14 changes: 9 additions & 5 deletions src/legacy/ui/public/agg_types/buckets/histogram.js
Original file line number Diff line number Diff line change
Expand Up @@ -76,13 +76,13 @@ export const histogramBucketAgg = new BucketAggType({
{
name: 'interval',
editorComponent: NumberIntervalParamEditor,
modifyAggConfigOnSearchRequestStart(aggConfig, searchSource) {
modifyAggConfigOnSearchRequestStart(aggConfig, searchSource, searchRequest) {
const field = aggConfig.getField();
const aggBody = field.scripted
? { script: { source: field.script, lang: field.lang } }
: { field: field.name };

return searchSource
const childSearchSource = searchSource
.createChild()
.setField('size', 0)
.setField('aggs', {
Expand All @@ -92,15 +92,19 @@ export const histogramBucketAgg = new BucketAggType({
minAgg: {
min: aggBody
}
})
.fetch()
});

searchRequest.whenAborted(() => childSearchSource.cancelQueued());

return childSearchSource.fetch()
.then((resp) => {
aggConfig.setAutoBounds({
min: _.get(resp, 'aggregations.minAgg.value'),
max: _.get(resp, 'aggregations.maxAgg.value')
});
})
.catch(() => {
.catch(e => {
if (e.name === 'AbortError') return;
toastNotifications.addWarning(i18n.translate('common.ui.aggTypes.histogram.missingMaxMinValuesWarning', {
// eslint-disable-next-line max-len
defaultMessage: 'Unable to retrieve max and min values to auto-scale histogram buckets. This may lead to poor visualization performance.'
Expand Down
5 changes: 4 additions & 1 deletion src/legacy/ui/public/agg_types/buckets/terms.js
Original file line number Diff line number Diff line change
Expand Up @@ -76,12 +76,15 @@ export const termsBucketAgg = new BucketAggType({
};
},
createFilter: createFilterTerms,
postFlightRequest: async (resp, aggConfigs, aggConfig, searchSource, inspectorAdapters) => {
postFlightRequest: async (resp, aggConfigs, aggConfig, searchSource, inspectorAdapters, abortSignal) => {
if (!resp.aggregations) return resp;
const nestedSearchSource = searchSource.createChild();
if (aggConfig.params.otherBucket) {
const filterAgg = buildOtherBucketAgg(aggConfigs, aggConfig, resp);
if (!filterAgg) return resp;
if (abortSignal) {
abortSignal.addEventListener('abort', () => nestedSearchSource.cancelQueued());
}

nestedSearchSource.setField('aggs', filterAgg);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -168,10 +168,13 @@ export function SearchRequestProvider(Promise) {

abort() {
this._markStopped();
this.defer = null;
this.aborted = true;
this.abortedDefer.resolve();
const error = new Error('The request was aborted.');
error.name = 'AbortError';
this.abortedDefer.resolve(error);
this.abortedDefer = null;
this.defer.reject(error);
this.defer = null;
}

whenAborted(cb) {
Expand Down
11 changes: 9 additions & 2 deletions src/legacy/ui/public/vis/request_handlers/courier.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ const CourierRequestHandlerProvider = function () {
partialRows,
metricsAtAllLevels,
inspectorAdapters,
queryFilter
queryFilter,
abortSignal,
}) {
// Create a new search source that inherits the original search source
// but has the appropriate timeRange applied via a filter.
Expand Down Expand Up @@ -101,6 +102,11 @@ const CourierRequestHandlerProvider = function () {
request.stats(getRequestInspectorStats(requestSearchSource));

try {
// Abort any in-progress requests before fetching again
if (abortSignal) {
abortSignal.addEventListener('abort', () => requestSearchSource.cancelQueued());
}

const response = await requestSearchSource.fetch();

searchSource.lastQuery = queryHash;
Expand Down Expand Up @@ -133,7 +139,8 @@ const CourierRequestHandlerProvider = function () {
aggs,
agg,
requestSearchSource,
inspectorAdapters
inspectorAdapters,
abortSignal,
);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ export interface RequestHandlerParams {
inspectorAdapters?: Adapters;
metricsAtAllLevels?: boolean;
visParams?: any;
abortSignal?: AbortSignal;
}

export type RequestHandler<T = unknown> = (params: RequestHandlerParams) => T;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -153,21 +153,30 @@ describe('EmbeddedVisualizeHandler', () => {
const params = { timeRange: { foo: 'bar' } };
handler.update(params);
jest.runAllTimers();
expect(mockDataLoaderFetch).toHaveBeenCalledWith({ ...dataLoaderParams, ...params });
expect(mockDataLoaderFetch).toHaveBeenCalled();
const { abortSignal, ...otherParams } = mockDataLoaderFetch.mock.calls[0][0];
expect(abortSignal).toBeInstanceOf(AbortSignal);
expect(otherParams).toEqual({ ...dataLoaderParams, ...params });
});

it('should call dataLoader.render with updated filters', () => {
const params = { filters: [{ meta: { disabled: false } }] };
handler.update(params);
jest.runAllTimers();
expect(mockDataLoaderFetch).toHaveBeenCalledWith({ ...dataLoaderParams, ...params });
expect(mockDataLoaderFetch).toHaveBeenCalled();
const { abortSignal, ...otherParams } = mockDataLoaderFetch.mock.calls[0][0];
expect(abortSignal).toBeInstanceOf(AbortSignal);
expect(otherParams).toEqual({ ...dataLoaderParams, ...params });
});

it('should call dataLoader.render with updated query', () => {
const params = { query: { foo: 'bar' } };
handler.update(params);
jest.runAllTimers();
expect(mockDataLoaderFetch).toHaveBeenCalledWith({ ...dataLoaderParams, ...params });
expect(mockDataLoaderFetch).toHaveBeenCalled();
const { abortSignal, ...otherParams } = mockDataLoaderFetch.mock.calls[0][0];
expect(abortSignal).toBeInstanceOf(AbortSignal);
expect(otherParams).toEqual({ ...dataLoaderParams, ...params });
});
});

Expand Down Expand Up @@ -197,6 +206,13 @@ describe('EmbeddedVisualizeHandler', () => {
handler.destroy();
expect(spy).toHaveBeenCalledTimes(1);
});

it('should call abort on controller', () => {
handler.abortController = new AbortController();
const spy = jest.spyOn(handler.abortController, 'abort');
handler.destroy();
expect(spy).toHaveBeenCalled();
});
});

describe('openInspector', () => {
Expand Down
Loading

0 comments on commit 9fecf8d

Please sign in to comment.