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 @@ -120,6 +120,7 @@

<dashboard-viewport-provider
get-embeddable-factory="getEmbeddableFactory"
abort-signal="abortSignal"
>
</dashboard-viewport-provider>

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)) {
// 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) {
timefilter.setTime(dateRange);
}
$scope.model.query = query;
dashboardStateManager.applyFilters($scope.model.query, $scope.model.filters);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ interface Props extends ReactIntl.InjectedIntlProps {
onPanelsUpdated: (updatedPanels: SavedDashboardPanelMap) => void;
maximizedPanelId?: string;
useMargins: boolean;
abortSignal?: AbortSignal;
}

interface State {
Expand Down Expand Up @@ -251,6 +252,7 @@ class DashboardGridUi extends React.Component<Props, State> {
<DashboardPanel
panelId={panel.panelIndex}
embeddableFactory={this.embeddableFactoryMap[panel.type]}
abortSignal={this.props.abortSignal}
onPanelFocused={this.onPanelFocused}
onPanelBlurred={this.onPanelBlurred}
/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ export interface DashboardPanelProps {
initialized: boolean;
panel: SavedDashboardPanel;
className?: string;
abortSignal?: AbortSignal;
}

export interface DashboardPanelUiProps extends DashboardPanelProps {
Expand Down Expand Up @@ -89,13 +90,14 @@ class DashboardPanelUi extends React.Component<DashboardPanelUiProps, State> {
embeddableStateChanged,
embeddableIsInitialized,
embeddableError,
abortSignal,
} = this.props;

if (!initialized) {
embeddableIsInitializing();
embeddableFactory
// @ts-ignore -- going away with Embeddable V2
.create(panel, embeddableStateChanged)
.create(panel, embeddableStateChanged, abortSignal)
.then((embeddable: Embeddable) => {
if (this.mounted) {
this.embeddable = embeddable;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ export function DashboardViewport({
useMargins,
isFullScreenMode,
onExitFullScreenMode,
abortSignal,
}: {
maximizedPanelId: string;
getEmbeddableFactory: (panelType: string) => EmbeddableFactory;
Expand All @@ -40,6 +41,7 @@ export function DashboardViewport({
useMargins: boolean;
isFullScreenMode: boolean;
onExitFullScreenMode: () => void;
abortSignal?: AbortSignal;
}) {
return (
<div
Expand All @@ -53,6 +55,7 @@ export function DashboardViewport({
<DashboardGrid
getEmbeddableFactory={getEmbeddableFactory}
maximizedPanelId={maximizedPanelId}
abortSignal={abortSignal}
/>
</div>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,4 +33,5 @@ export function DashboardViewportProvider(props) {

DashboardViewportProvider.propTypes = {
getEmbeddableFactory: PropTypes.func.isRequired,
abortSignal: PropTypes.object,
};
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ export interface VisualizeEmbeddableConfiguration {
editUrl?: string;
editable: boolean;
loader: VisualizeLoader;
abortSignal?: AbortSignal;
}

export class VisualizeEmbeddable extends Embeddable {
Expand All @@ -54,6 +55,7 @@ export class VisualizeEmbeddable extends Embeddable {
private timeRange?: TimeRange;
private query?: Query;
private filters?: Filter[];
private abortSignal?: AbortSignal;

constructor({
onEmbeddableStateChanged,
Expand All @@ -62,6 +64,7 @@ export class VisualizeEmbeddable extends Embeddable {
editUrl,
editable,
loader,
abortSignal,
}: VisualizeEmbeddableConfiguration) {
super({
title: savedVisualization.title,
Expand All @@ -75,6 +78,7 @@ export class VisualizeEmbeddable extends Embeddable {
this.onEmbeddableStateChanged = onEmbeddableStateChanged;
this.savedVisualization = savedVisualization;
this.loader = loader;
this.abortSignal = abortSignal;

const parsedUiState = savedVisualization.uiStateJSON
? JSON.parse(savedVisualization.uiStateJSON)
Expand Down Expand Up @@ -184,6 +188,7 @@ export class VisualizeEmbeddable extends Embeddable {
filters: containerState.filters,
cssClass: `embPanel__content embPanel__content--fullWidth`,
dataAttrs,
abortSignal: this.abortSignal,
};

this.handler = this.loader.embedVisualizationWithSavedObject(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,8 @@ export class VisualizeEmbeddableFactory extends EmbeddableFactory<VisualizationA
*/
public async create(
panelMetadata: EmbeddableInstanceConfiguration,
onEmbeddableStateChanged: OnEmbeddableStateChanged
onEmbeddableStateChanged: OnEmbeddableStateChanged,
abortSignal?: AbortSignal
) {
const visId = panelMetadata.id;
const editUrl = this.getEditPath(visId);
Expand All @@ -119,6 +120,7 @@ export class VisualizeEmbeddableFactory extends EmbeddableFactory<VisualizationA
editable,
loader,
indexPatterns,
abortSignal,
});
}
}
7 changes: 6 additions & 1 deletion src/legacy/ui/public/embeddable/embeddable_factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ export type OnEmbeddableStateChanged = (embeddableStateChanges: EmbeddableState)
export abstract class EmbeddableFactory<T extends SavedObjectAttributes = SavedObjectAttributes> {
public readonly name: string;
public readonly savedObjectMetaData?: SavedObjectMetaData<T>;
protected abortSignal?: AbortSignal;

/**
*
Expand All @@ -42,12 +43,15 @@ export abstract class EmbeddableFactory<T extends SavedObjectAttributes = SavedO
constructor({
name,
savedObjectMetaData,
abortSignal,
}: {
name: string;
savedObjectMetaData?: SavedObjectMetaData<T>;
abortSignal?: AbortSignal;
}) {
this.name = name;
this.savedObjectMetaData = savedObjectMetaData;
this.abortSignal = abortSignal;
}

/**
Expand All @@ -61,6 +65,7 @@ export abstract class EmbeddableFactory<T extends SavedObjectAttributes = SavedO
*/
public abstract create(
containerMetadata: { id: string },
onEmbeddableStateChanged: OnEmbeddableStateChanged
onEmbeddableStateChanged: OnEmbeddableStateChanged,
abortSignal?: AbortSignal
): Promise<Embeddable>;
}
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