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

cancellation of interpreter execution #40238

Merged
merged 11 commits into from
Jul 30, 2019
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ export const esaggs = (): ExpressionFunction<typeof name, Context, Arguments, Re
partialRows: args.partialRows,
inspectorAdapters: handlers.inspectorAdapters,
queryFilter,
abortSignal: handlers.abortSignal,
});

return {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -249,17 +249,15 @@ export class DashboardAppController {
!dashboardConfig.getHideWriteControls();

$scope.updateQueryAndFetch = function({ query, dateRange }) {
if (dateRange) {
timefilter.setTime(dateRange);
}

const oldQuery = $scope.model.query;
if (_.isEqual(oldQuery, query)) {
if (_.isEqual($scope.model.query, query) && _.isEqual($scope.model.timeRange, dateRange)) {
lukasolson marked this conversation as resolved.
Show resolved Hide resolved
// The user can still request a reload in the query bar, even if the
// query is the same, and in that case, we have to explicitly ask for
// a reload, since no state changes will cause it.
dashboardStateManager.requestReload();
} else {
if (dateRange) {
lukasolson marked this conversation as resolved.
Show resolved Hide resolved
timefilter.setTime(dateRange);
}
$scope.model.query = query;
dashboardStateManager.applyFilters($scope.model.query, $scope.model.filters);
}
Expand Down
13 changes: 5 additions & 8 deletions src/legacy/ui/public/vis/editors/default/default.js
Original file line number Diff line number Diff line change
Expand Up @@ -164,18 +164,15 @@ const defaultEditor = function ($rootScope, $compile) {
}
this._loader = loader;
this._handler = this._loader.embedVisualizationWithSavedObject(visualizationEl, this.savedObj, {
uiState: uiState,
listenOnChange: false,
timeRange: timeRange,
filters: filters,
appState: appState,
uiState,
timeRange,
filters,
appState,
});
});
} else {
this._handler.update({
timeRange: timeRange,
filters: filters,
});
this._handler.update({ timeRange, filters });
}

});
Expand Down
9 changes: 7 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,9 +40,9 @@ 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.
// This is a temporary solution until we properly pass down all required
Expand Down Expand Up @@ -102,6 +102,11 @@ const CourierRequestHandlerProvider = function () {
request.stats(getRequestInspectorStats(requestSearchSource));

try {
if (abortSignal) {
abortSignal.addEventListener('abort', () => {
requestSearchSource.cancelQueued();
Copy link
Member Author

Choose a reason for hiding this comment

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

what will this actually do ?

  • cancel queued (so requests that didn't start yet ?)
  • cancel current requests ?
  • on search source or on courier level ? (what happens if the request was batched with another one ?)

Copy link
Member

Choose a reason for hiding this comment

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

It aborts all incomplete requests for this searchSource object. In practice, because of Courier, requests from different searchSource objects are batched up, and cancelling one will cancel all that are bundled within the same _msearch. I don't think this is necessarily a bad thing because it will probably always be what we want. For folks that disable batching, this will only cancel the request for this specific searchSource.

});
}
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>(params: RequestHandlerParams) => T;
Expand Down
25 changes: 11 additions & 14 deletions src/legacy/ui/public/visualize/loader/embedded_visualize_handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,12 @@ export class EmbeddedVisualizeHandler {

const forceFetch = this.shouldForceNextFetch;
this.shouldForceNextFetch = false;

// Abort any in-progress requests
if (this.abortController) this.abortController.abort();
this.abortController = new AbortController();
this.dataLoaderParams.abortSignal = this.abortController.signal;

this.fetch(forceFetch).then(this.render);
}, 100);

Expand All @@ -104,6 +110,7 @@ export class EmbeddedVisualizeHandler {
private actions: any = {};
private events$: Rx.Observable<any>;
private autoFetch: boolean;
private abortController?: AbortController;

constructor(
private readonly element: HTMLElement,
Expand Down Expand Up @@ -223,7 +230,7 @@ export class EmbeddedVisualizeHandler {
*/
public update(params: VisualizeUpdateParams = {}) {
// Apply data- attributes to the element if specified
const dataAttrs = params.dataAttrs;
const { dataAttrs, ...otherParams } = params;
if (dataAttrs) {
Object.keys(dataAttrs).forEach(key => {
if (dataAttrs[key] === null) {
Expand All @@ -235,19 +242,8 @@ export class EmbeddedVisualizeHandler {
});
}

let fetchRequired = false;
if (params.hasOwnProperty('timeRange')) {
fetchRequired = true;
this.dataLoaderParams.timeRange = params.timeRange;
}
if (params.hasOwnProperty('filters')) {
fetchRequired = true;
this.dataLoaderParams.filters = params.filters;
}
if (params.hasOwnProperty('query')) {
fetchRequired = true;
this.dataLoaderParams.query = params.query;
}
const fetchRequired = ['timeRange', 'filters', 'query'].some(key => params.hasOwnProperty(key));
this.dataLoaderParams = { ...this.dataLoaderParams, ...otherParams };
Copy link
Member

Choose a reason for hiding this comment

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

The reason this was changed is because I had originally added 'abortSignal' in here, and didn't feel like copying/pasting the exact same code 4 times made sense when we could just do it like this. Then I ended up removing 'abortSignal' from here and actually creating it in the handler. I don't mind reverting these changes if it makes people feel more comfortable :)


if (fetchRequired) {
this.fetchAndRender();
Expand All @@ -260,6 +256,7 @@ export class EmbeddedVisualizeHandler {
*/
public destroy(): void {
this.destroyed = true;
if (this.abortController) this.abortController.abort();
this.debouncedFetchAndRender.cancel();
if (this.autoFetch) {
timefilter.off('autoRefreshFetch', this.reload);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ export class PipelineDataLoader {
: undefined,
}),
inspectorAdapters: params.inspectorAdapters,
abortSignal: params.abortSignal,
}
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ type getInitialContextFunction = () => KibanaContext;
export interface RunPipelineHandlers {
getInitialContext: getInitialContextFunction;
inspectorAdapters?: Adapters;
abortSignal?: AbortSignal;
}

export const runPipeline = async (
Expand Down
5 changes: 5 additions & 0 deletions src/legacy/ui/public/visualize/loader/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,11 @@ export interface VisualizeLoaderParams {
* cycle happens. Default value: `true`
*/
autoFetch?: boolean;

/**
* AbortSignal can be provided which allows canceling ongoing process
*/
abortSignal?: AbortSignal;
}

/**
Expand Down
5 changes: 5 additions & 0 deletions src/plugins/data/common/expressions/interpreter_provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,11 @@ export function interpreterProvider(config: any) {
// if something failed, just return the failure
if (getType(newContext) === 'error') return newContext;

// if execution was aborted return error
if (handlers.abortSignal && handlers.abortSignal.aborted) {
return new Error(`abort was requested`);
Copy link
Contributor

@streamich streamich Jul 9, 2019

Choose a reason for hiding this comment

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

Maybe something with word expression in it, like

new Error('Expression was aborted.');

}

// Continue re-invoking chain until it's empty
return await invokeChain(chain, newContext);
} catch (e) {
Expand Down