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

Cancel visualize fetches when navigating away or re-fetching #42035

Merged
merged 33 commits into from
Aug 28, 2019
Merged
Show file tree
Hide file tree
Changes from 32 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
802f608
add AbortSignal to interpreter
ppisljar Jul 3, 2019
daf72d2
adding AbortSignal to visualize_loader
ppisljar Jul 3, 2019
332dae1
adding AbortSignal to embeddables and dashboard
ppisljar Jul 3, 2019
04e1240
passing AbortSignal to courier request handler
ppisljar Jul 3, 2019
0c429c1
Remove abort signal from dashboard and move to handler, and abort fet…
lukasolson Jul 9, 2019
095860e
Remove the rest of the references to abort signal in dashboard
lukasolson Jul 9, 2019
3c7e344
Revert changes to dashboard_app
lukasolson Jul 10, 2019
4af0b64
Remove code related to canceling visualize requests and only keep stu…
lukasolson Jul 16, 2019
89ebc13
Merge branch 'master' into interpreter/canceling
lukasolson Jul 24, 2019
d8f5208
Use createError
lukasolson Jul 25, 2019
00a672d
Cancel in-progress fetches when a new one is requested or when leavin…
lukasolson Jul 25, 2019
1d7f782
Merge branch 'master' into cancelVisualizeRequests
lukasolson Jul 31, 2019
b37525f
Update with cancel methods and make visualize data loader work
lukasolson Jul 31, 2019
dd784a3
Remove unnecessary call to
lukasolson Aug 1, 2019
cb2fb2d
Fix tests
lukasolson Aug 2, 2019
daa3a48
Merge branch 'master' of github.com:elastic/kibana into cancelVisuali…
lukasolson Aug 2, 2019
897cd96
Merge branch 'master' into cancelVisualizeRequests
lukasolson Aug 6, 2019
3f0b86f
Remove cancel from data loaders
lukasolson Aug 7, 2019
5ddfe24
Merge branch 'cancelVisualizeRequests' of github.com:lukasolson/kiban…
lukasolson Aug 7, 2019
b1ec8e4
Fix search source to always either resolve or reject in fetch()
lukasolson Aug 8, 2019
215be5d
Fix naming
lukasolson Aug 8, 2019
d8535d4
Merge branch 'master' into cancelVisualizeRequests
lukasolson Aug 8, 2019
d63a824
Update search request to reject on abort, and don't render errors in …
lukasolson Aug 8, 2019
aeb5045
Update tests
lukasolson Aug 9, 2019
726c4a0
Merge branch 'master' into cancelVisualizeRequests
lukasolson Aug 20, 2019
8255d2f
Merge branch 'master' into cancelVisualizeRequests
lukasolson Aug 23, 2019
6492023
Merge branch 'master' into cancelVisualizeRequests
lukasolson Aug 27, 2019
728a7b4
Add cancellation to input control
lukasolson Aug 27, 2019
e068744
Cancel histogram fetches
lukasolson Aug 27, 2019
72a8965
Update esaggs typings
lukasolson Aug 27, 2019
33180fb
Merge branch 'master' into cancelVisualizeRequests
lukasolson Aug 28, 2019
d5983e4
Add cancellation for search embeddables
lukasolson Aug 28, 2019
4949fb4
Abort post-flight requests
lukasolson Aug 28, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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) {
Copy link
Member

Choose a reason for hiding this comment

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

can we rather pass abortSignal in here to have same implementation as in other places ?( abortSignal.addEventListener)

Copy link
Member Author

@lukasolson lukasolson Aug 28, 2019

Choose a reason for hiding this comment

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

This won't quite work since this method is called from both buildVislibDimensions (which has access to the abortSignal but not the searchRequest) and searchSource.requestIsStarting (which has access to the searchRequest but not the abortSignal). I could theoretically pass in abortSignal as an additional argument, then check both searchRequest and abortSignal but that seems kinda messy. Any other proposals?

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
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
8 changes: 7 additions & 1 deletion 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());
lukasolson marked this conversation as resolved.
Show resolved Hide resolved
}

const response = await requestSearchSource.fetch();

searchSource.lastQuery = queryHash;
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
50 changes: 33 additions & 17 deletions src/legacy/ui/public/visualize/loader/embedded_visualize_handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ export class EmbeddedVisualizeHandler {
private actions: any = {};
private events$: Rx.Observable<any>;
private autoFetch: boolean;
private abortController?: AbortController;
private autoRefreshFetchSubscription: Rx.Subscription | undefined;

constructor(
Expand Down Expand Up @@ -268,6 +269,7 @@ export class EmbeddedVisualizeHandler {
*/
public destroy(): void {
this.destroyed = true;
this.cancel();
this.debouncedFetchAndRender.cancel();
if (this.autoFetch) {
if (this.autoRefreshFetchSubscription) this.autoRefreshFetchSubscription.unsubscribe();
Expand Down Expand Up @@ -443,7 +445,14 @@ export class EmbeddedVisualizeHandler {
this.fetchAndRender();
};

private cancel = () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Should this be public?

Copy link
Member

Choose a reason for hiding this comment

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

yup, i think so, but not necessary for now
we'll want to cancel externally (once we have cancel buttons around)

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

private fetch = (forceFetch: boolean = false) => {
this.cancel();
lukasolson marked this conversation as resolved.
Show resolved Hide resolved
this.abortController = new AbortController();
this.dataLoaderParams.abortSignal = this.abortController.signal;
this.dataLoaderParams.aggs = this.vis.getAggConfig();
this.dataLoaderParams.forceFetch = forceFetch;
this.dataLoaderParams.inspectorAdapters = this.inspectorAdapters;
Expand All @@ -452,23 +461,27 @@ export class EmbeddedVisualizeHandler {
this.vis.requestError = undefined;
this.vis.showRequestError = false;

return this.dataLoader
.fetch(this.dataLoaderParams)
.then(data => {
// Pipeline responses never throw errors, so we need to check for
// `type: 'error'`, and then throw so it can be caught below.
// TODO: We should revisit this after we have fully migrated
// to the new expression pipeline infrastructure.
if (data && data.type === 'error') {
throw data.error;
}

if (data && data.value) {
this.dataSubject.next(data.value);
}
return data;
})
.catch(this.handleDataLoaderError);
return (
this.dataLoader
// Don't pass in this.dataLoaderParams directly because it may be modified async in another
// call to fetch before the previous one has completed
.fetch({ ...this.dataLoaderParams })
.then(data => {
// Pipeline responses never throw errors, so we need to check for
// `type: 'error'`, and then throw so it can be caught below.
// TODO: We should revisit this after we have fully migrated
// to the new expression pipeline infrastructure.
if (data && data.type === 'error') {
throw data.error;
}

if (data && data.value) {
this.dataSubject.next(data.value);
}
return data;
})
.catch(this.handleDataLoaderError)
);
};

/**
Expand All @@ -478,6 +491,9 @@ export class EmbeddedVisualizeHandler {
* frequently encountered by users.
*/
private handleDataLoaderError = (error: any): void => {
// If the data loader was aborted then no need to surface this error in the UI
if (error && error.name === 'AbortError') return;

// TODO: come up with a general way to cancel execution of pipeline expressions.
if (this.dataLoaderParams.searchSource && this.dataLoaderParams.searchSource.cancelQueued) {
this.dataLoaderParams.searchSource.cancelQueued();
Expand Down
Loading