cancellation of interpreter execution #40238

ppisljar · 2019-07-03T08:45:44Z

Summary

Adds cancelation support to interpreter via AbortSignal

AbortSignal is checked on every step of chain, if abort was requested interpreter will return with error and will not continue executing any remaining functions.
AbortSignal is also passed to every function as one of the handlers. This way functions that take long time can also cancel early.

Dev Docs

The interpreter now accepts an AbortController signal inside handlers for aborting execution. Since handlers is passed down to each function in the interpreter, functions themselves can handle additional cleanup using handlers.abortSignal if it is available.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

This was checked for cross-browser compatibility, including a check against IE11
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

elasticmachine · 2019-07-03T08:46:05Z

Pinging @elastic/kibana-app-arch

elasticmachine · 2019-07-03T09:43:35Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

lukasolson · 2019-07-03T21:59:40Z

src/legacy/core_plugins/kibana/public/dashboard/dashboard_app_controller.tsx

@@ -128,6 +128,8 @@ export class DashboardAppController {
    const panelActionsRegistry = Private(ContextMenuActionsRegistryProvider);
    const getUnhashableStates = Private(getUnhashableStatesProvider);
    const shareContextMenuExtensions = Private(ShareContextMenuExtensionsRegistryProvider);
+    const abortController = new AbortController();


I think you're going to want to import abortcontroller-polyfill here as some browsers don't support this natively.

Also I think you're going to want to create a new AbortController for each update, not just one on the controller load.

@lukasolson I had thought about the polyfill question too, because I was worried that any plugin dev who wanted to use this feature would need to deal with the polyfill. But then I noticed we are importing it here: https://github.com/elastic/kibana/blob/master/src/legacy/ui/ui_bundles/app_entry_template.js#L40

I'm not super familiar with how the entry template works, but I assume this means we won't need to re-import that polyfill elsewhere in kibana?

Yeah, you could totally be right. I'll give this PR a spin on IE just to make sure.

lukeelmers · 2019-07-03T22:50:23Z

src/legacy/core_plugins/kibana/public/dashboard/dashboard_app_controller.tsx

@@ -128,6 +128,8 @@ export class DashboardAppController {
    const panelActionsRegistry = Private(ContextMenuActionsRegistryProvider);
    const getUnhashableStates = Private(getUnhashableStatesProvider);
    const shareContextMenuExtensions = Private(ShareContextMenuExtensionsRegistryProvider);
+    const abortController = new AbortController();


@lukasolson I had thought about the polyfill question too, because I was worried that any plugin dev who wanted to use this feature would need to deal with the polyfill. But then I noticed we are importing it here: https://github.com/elastic/kibana/blob/master/src/legacy/ui/ui_bundles/app_entry_template.js#L40

I'm not super familiar with how the entry template works, but I assume this means we won't need to re-import that polyfill elsewhere in kibana?

lukeelmers · 2019-07-03T22:57:14Z

src/plugins/data/common/expressions/interpreter_provider.ts

@@ -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 createError(`abort was requested`);


createError is surfaced in the UI... is this definitely the behavior we want? Like, if we add a button for someone to cancel, do we want to always see an error message, since technically this would be the expected behavior from a user perspective? (Or maybe it should be a confirmation/success message?)

Also, I'm wondering if it would be better to throw an error here instead, and catch it below? That way all of our error handling happens in one place.

Agreed, I've changed it to a simple Error in 0c429c1. We should probably eventually make a separate AbortedError or something of the sort.

…ches when necessary

lukasolson · 2019-07-09T19:39:41Z

I added a few commits here, I don't think we need to have the dashboard itself be responsible for creating the abort signal. I think it makes more sense to have it in the handler itself, seeing as how it determines when a fetch is necessary. It also has a destroy method that the dashboard uses (and visualize) that can also abort. @ppisljar @lukeelmers Let me know what you think of the recent changes. P.S. This is now actually aborting requests in visualize embeddables (both in Visualize and Dashboard).

elasticmachine · 2019-07-09T20:26:21Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

streamich · 2019-07-09T21:07:22Z

src/plugins/data/common/expressions/interpreter_provider.ts

@@ -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`);


Maybe something with word expression in it, like

new Error('Expression was aborted.');

lukeelmers · 2019-07-09T22:28:37Z

I think it makes more sense to have it in the handler itself

yeah this makes sense to me too... it feels like a simpler place to start having it centralized in one place, and also being able to rely on the existing destroy method in the embedded visualize handler

ppisljar

regarding moving to loader: then the only way to stop the request is to destroy the loader (embeddable)
which is probably not what we want. If i am looking at the dashboard, and i enter a query, then before i got response i enter another query (very easy ... i enter 20, press enter, i add another 0, to get 200 and press enter) ... now all my embeddables would get destroyed, instead of just updated.

i think we need to make difference between aborting requests and destroying. This doens't mean however that the logic for this can't live in the loader. But on the other hand, lets not forget that we'll want to cancel more than just visualization requests, and long term in most places we won't be using visualize loader anyway.

src/legacy/core_plugins/kibana/public/dashboard/dashboard_app_controller.tsx

ppisljar · 2019-07-10T07:42:00Z

src/legacy/ui/public/vis/request_handlers/courier.js

@@ -102,6 +102,11 @@ const CourierRequestHandlerProvider = function () {
        request.stats(getRequestInspectorStats(requestSearchSource));

        try {
+          if (abortSignal) {
+            abortSignal.addEventListener('abort', () => {
+              requestSearchSource.cancelQueued();


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 ?)

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.

lukasolson · 2019-07-10T14:58:36Z

which is probably not what we want. If i am looking at the dashboard, and i enter a query, then before i got response i enter another query (very easy ... i enter 20, press enter, i add another 0, to get 200 and press enter) ... now all my embeddables would get destroyed, instead of just updated.

This isn't quite true... In the PR you can see that we are aborting both when the handler is destroyed as well as when a new fetch is requested.

lukasolson · 2019-07-10T15:16:19Z

src/legacy/ui/public/visualize/loader/embedded_visualize_handler.ts

-      this.dataLoaderParams.query = params.query;
-    }
+    const fetchRequired = ['timeRange', 'filters', 'query'].some(key => params.hasOwnProperty(key));
+    this.dataLoaderParams = { ...this.dataLoaderParams, ...otherParams };


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 :)

lukasolson · 2019-07-10T15:33:48Z

But on the other hand, lets not forget that we'll want to cancel more than just visualization requests, and long term in most places we won't be using visualize loader anyway.

Totally agreed. I still don't think it makes sense to have the dashboard be responsible for creating the signal and passing it through the dashboard grid/panel/viewport, to the embeddable/handler/loader, etc. The dashboard is responsible for managing state and passing it down to the embeddable. The embeddable/handler is responsible for handling state changes, and determining whether or not a fetch is needed (and in turn the expression is interpreted), and whether or not an existing fetch should be cancelled. Other places that want to cancel the interpreter execution can do things the same way.

elasticmachine · 2019-07-10T16:58:48Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

ppisljar · 2019-07-11T06:58:18Z

there is still a question on how to handle user aborts.

loader.destroy() cancels queries when we navigate away
loader.update() cancels queries, when we want to refetch

we still need something to handle user clicking on a cancel button (at some point we will provide it).
calling destroy() or update() sounds wrong, we would need something like .cancel() on the loader, which will cancel the existing requests, but won't destroy visualization (so you will still look at the latest render) or we need to pass abort signal to it.

if we decide to expose yet another method, it doesn't need to be part of this PR until we actually need it, but i think we should make the decision on how to handle it now, as if we decide to pass abortsignal in instead of exposing cancel method, that will affect other methods as well (most likely).

any strong opinions about having it either way ?

cc @timroes @joshdover just pinging you here to see whats your opinion on the matter.

lukasolson · 2019-07-11T21:09:42Z

Just wanted to also comment loader.reload() will also cancel existing fetches. To me it makes sense to add a cancel() method, similar to how we added the reload method because it was necessary, but I'm open to hearing other possible solutions.

lukeelmers · 2019-07-11T22:36:04Z

if we decide to expose yet another method, it doesn't need to be part of this PR until we actually need it, but i think we should make the decision on how to handle it now

Yeah I agree with this; if we don't need it now, let's not expose it. And since we're going to go down the path of exposing a way to manually cancel eventually, then I suppose you are right it makes more sense to put it closer to where we expect it to live in the future, vs in the request handler where would have to be later removed anyway.

To me it makes sense to add a cancel() method, similar to how we added the reload method

++ I totally agree this feels like the natural place to put this if you want to expose a method for users to initiate a cancellation -- so you have reload, destroy, cancel, update, etc.

I suppose the more fundamental design question @ppisljar was getting at is: do we want AbortSignal to be a "thing" app devs must use when building stuff in Kibana, or do we want to abstract that away behind an interface of some sort?

I see pros and cons to each approach; what's probably more important is keeping consistency in our implementations across various services (especially core + app arch services).

lukasolson · 2019-07-11T23:42:00Z

I suppose the more fundamental design question @ppisljar was getting at is: do we want AbortSignal to be a "thing" app devs must use when building stuff in Kibana, or do we want to abstract that away behind an interface of some sort?

Yeah, that makes sense. I think having these methods instead of passing in an AbortSignal is desirable. Having an abort signal means you can only listen to when the signal is aborted or read its state (aborted or not). However, if you have the reload, cancel, update, etc. methods, and can create an abort controller/signal in some of these methods, it gives a lot more flexibility. If the loader itself only got an abort signal, it couldn't programmatically abort, but instead would have to rely on something above in the chain aborting. I'm not sure where that would be...

An update, reload, or destroy might not always mean "abort what's already in progress" (although, to be fair, cancel probably does). Having the flexibility to determine if/when they do abort seems beneficial.

For example, say I have an embeddable that actually creates new documents in Elasticsearch. If I submit a request to create a new document, then navigate away before the request completes, I might not actually want the request to be aborted in the destroy method, yet if all I get is an abort signal that I pass on to my Elasticsearch request, that's what will happen.

…ff for canceling interpreter

lukasolson · 2019-07-16T16:33:24Z

As I was trimming down this PR to not include the stuff related to actually aborting visualize loading (which will come in a separate PR) I was thinking, abortSignal isn't really something that belongs in the handlers, because handlers is initialized prior to the interpreter being initialized. If it's in handlers, then as soon as you abort(), then any future calls to interpret will return an abort error. If we add it as an additional argument to interpret, I think it makes a lot more sense, because then you can control aborting at a more granular level (individual calls to interpret).

Something like this:

async function invokeChain(chainArr: any, context: any, abortSignal: AbortSignal): Promise<any> {
    if (!chainArr.length) return Promise.resolve(context);

    // if execution was aborted return error
    if (abortSignal && abortSignal.aborted) {
      return new Error('Expression was aborted.');
    }

   ...

I'm going to move forward with this approach unless someone suggests differently.

/cc @lukeelmers @ppisljar @streamich

elasticmachine · 2019-07-16T17:30:45Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

lukeelmers · 2019-07-18T21:36:35Z

(@lukasolson summarizing our offline conversation)

Since the InterpreterProvider is called every time we call the interpret fn (via interpretAst), then any provided handlers (or a separate abortSignal param as you've mentioned) will be given to the individual expression you are wanting to interpret.

So cancellation works either way, whether the signal is in handlers or as its own param.

But the one thing we'll have to solve in either case is how we deal with this in embedded_visualize_handler, which is instantiated once but can trigger multiple calls to the interpreter as params are changed. Presumably this would be via EmbeddedVisualizeHandler.update() and EmbeddedVisualizeHandler.reload()

elasticmachine · 2019-07-25T00:41:08Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

lukasolson · 2019-07-25T19:47:32Z

src/plugins/data/common/expressions/interpreter_provider.ts

@@ -71,8 +71,16 @@ 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 createError({


Moved this back to being createError because of the comment below:

The interpreter should never fail. It should always return a {type: error} on failure

The consumers can check if err.name === 'AbortError' and handle those failures differently than other failures.

src/plugins/data/common/expressions/interpreter_provider.ts

lukasolson · 2019-07-25T19:58:22Z

@ppisljar @lukeelmers I've trimmed this PR down specifically to just be the interpreter portion, and will create a new PR that relies on this one to actually pass in abortSignal inside visualize embeddables, and handles it inside esaggs.

elasticmachine · 2019-07-25T20:41:46Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…anceling

elasticmachine · 2019-07-30T14:33:29Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

lukasolson · 2019-07-30T16:36:35Z

Retest

elasticmachine · 2019-07-30T17:37:59Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

lukasolson · 2019-07-30T23:50:10Z

@ppisljar gave this the LGTM in a message to me, because he couldn't "approve" of his own PR 😄

* 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

lukasolson · 2019-07-30T23:56:04Z

7.x (7.4.0): 3ccc9a9

…-or-edit-existing-rollup-job * 'master' of github.com:elastic/kibana: (114 commits) [ML] Fixing empty index pattern list (elastic#42299) [Markdown] Shim new platform - cleanup plugin (elastic#41760) [Code] Enable hierarchicalDocumentSymbolSupport for java language server (elastic#42233) Add New Platform mocks for data plugin (elastic#42261) Http server route handler implementation (elastic#41894) [SR] Allow custom index pattern to be used instead of selectable list when choosing indices to restore (elastic#41534) [Code] distributed Code abstraction (elastic#41374) [SIEM] Sets page titles to the current page you are on (elastic#42157) Saved Objects export API stable type order (elastic#42310) cancellation of interpreter execution (elastic#40238) [SIEM] Fixes a crash when Machine Learning influencers is an undefined value (elastic#42198) Changed the job to work with a dedicated index (elastic#42297) FTR: fix testSubjects.missingOrFail (elastic#42290) Increase retry timeout to prevent flaky tests (elastic#42291) Spaces - make space a hidden saved object type (elastic#41688) Allow applications to register feature privileges which are excluded from the base privileges (elastic#41300) Disable flaky log column reorder test (elastic#42285) Fixing add element in element reducer (elastic#42276) Fix infinite loop (elastic#42228) [Maps][File upload] Remove geojson deep clone logic, handle on maps side (elastic#41835) ...

ppisljar added 4 commits July 3, 2019 10:37

add AbortSignal to interpreter

802f608

adding AbortSignal to visualize_loader

daf72d2

adding AbortSignal to embeddables and dashboard

332dae1

passing AbortSignal to courier request handler

04e1240

ppisljar requested review from lukasolson and lukeelmers July 3, 2019 08:45

ppisljar added Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) Team:AppArch v7.4.0 v8.0.0 labels Jul 3, 2019

lukasolson reviewed Jul 3, 2019

View reviewed changes

lukeelmers reviewed Jul 3, 2019

View reviewed changes

lukasolson added 2 commits July 9, 2019 12:19

Remove abort signal from dashboard and move to handler, and abort fet…

0c429c1

…ches when necessary

Remove the rest of the references to abort signal in dashboard

095860e

streamich reviewed Jul 9, 2019

View reviewed changes

ppisljar commented Jul 10, 2019

View reviewed changes

lukasolson reviewed Jul 10, 2019

View reviewed changes

Revert changes to dashboard_app

3c7e344

lukasolson self-assigned this Jul 16, 2019

Remove code related to canceling visualize requests and only keep stu…

4af0b64

…ff for canceling interpreter

Merge branch 'master' into interpreter/canceling

89ebc13

Use createError

d8f5208

lukasolson reviewed Jul 25, 2019

View reviewed changes

src/plugins/data/common/expressions/interpreter_provider.ts Show resolved Hide resolved

lukasolson marked this pull request as ready for review July 25, 2019 19:56

lukasolson requested a review from a team as a code owner July 25, 2019 19:56

lukasolson mentioned this pull request Jul 25, 2019

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

Merged

7 tasks

Merge branch 'master' of github.com:elastic/kibana into interpreter/c…

301c7f1

…anceling

lukasolson added the release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. label Jul 30, 2019

lukasolson approved these changes Jul 30, 2019

View reviewed changes

lukasolson merged commit 305e4a2 into elastic:master Jul 30, 2019

lukasolson mentioned this pull request Sep 16, 2019

[Interpreter] Make interpreter execution cancellable #39962

Closed

lukasolson mentioned this pull request Sep 26, 2019

Refactor, deangularize courier/search source #45235

Merged

4 tasks

cancellation of interpreter execution #40238

cancellation of interpreter execution #40238

Conversation

ppisljar commented Jul 3, 2019 • edited by lukasolson Loading

Summary

Dev Docs

Checklist

For maintainers

elasticmachine commented Jul 3, 2019

elasticmachine commented Jul 3, 2019

💚 Build Succeeded

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukasolson commented Jul 9, 2019

elasticmachine commented Jul 9, 2019

💔 Build Failed

streamich Jul 9, 2019 • edited Loading

Choose a reason for hiding this comment

lukeelmers commented Jul 9, 2019

ppisljar left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukasolson commented Jul 10, 2019

Choose a reason for hiding this comment

lukasolson commented Jul 10, 2019

elasticmachine commented Jul 10, 2019

💔 Build Failed

ppisljar commented Jul 11, 2019 • edited Loading

lukasolson commented Jul 11, 2019

lukeelmers commented Jul 11, 2019

lukasolson commented Jul 11, 2019

lukasolson commented Jul 16, 2019

elasticmachine commented Jul 16, 2019

💔 Build Failed

lukeelmers commented Jul 18, 2019

elasticmachine commented Jul 25, 2019

💚 Build Succeeded

Choose a reason for hiding this comment

lukasolson commented Jul 25, 2019

elasticmachine commented Jul 25, 2019

💔 Build Failed

elasticmachine commented Jul 30, 2019

💔 Build Failed

lukasolson commented Jul 30, 2019

elasticmachine commented Jul 30, 2019

💚 Build Succeeded

lukasolson commented Jul 30, 2019

lukasolson commented Jul 30, 2019

ppisljar commented Jul 3, 2019 •

edited by lukasolson

Loading

streamich Jul 9, 2019 •

edited

Loading

ppisljar left a comment •

edited

Loading

ppisljar commented Jul 11, 2019 •

edited

Loading