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

Abort cancelled search requests to Elasticsearch #56788

Merged
merged 36 commits into from
Feb 25, 2020
Merged
Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
f15f138
Update abort controller library
lukasolson Feb 3, 2020
4d36124
Bootstrap
lukasolson Feb 3, 2020
1943855
Merge branch 'master' into abortController
elasticmachine Feb 3, 2020
491e8bb
Abort when the request is aborted
lukasolson Feb 4, 2020
d5d995c
Merge branch 'master' into abortController
lukasolson Feb 4, 2020
8afc936
Merge branch 'abortController' of github.com:lukasolson/kibana into a…
lukasolson Feb 4, 2020
48238d8
Merge branch 'abortController' into abortSearch
lukasolson Feb 4, 2020
0db7257
Add utility and update value suggestions route
lukasolson Feb 4, 2020
f376da7
Merge branch 'master' into abortSearch
lukasolson Feb 5, 2020
ad5f188
Remove bad merge
lukasolson Feb 6, 2020
48b88c0
Revert switching abort controller libraries
lukasolson Feb 10, 2020
21dd1c8
Revert package.json in lib
lukasolson Feb 10, 2020
16064ff
Merge branch 'revertAbortController' into abortSearch
lukasolson Feb 10, 2020
a079c15
Merge branch 'master' into revertAbortController
elasticmachine Feb 10, 2020
0b87a0b
Move to previous abort controller
lukasolson Feb 10, 2020
55d76f3
Merge branch 'master' into abortSearch
lukasolson Feb 10, 2020
8292c1e
Merge branch 'master' into revertAbortController
lukasolson Feb 10, 2020
e45bdd6
Merge remote-tracking branch 'origin/revertAbortController' into reve…
lukasolson Feb 10, 2020
d3291a8
Merge branch 'master' into revertAbortController
elasticmachine Feb 12, 2020
f996a65
Merge branch 'master' into abortSearch
lukasolson Feb 12, 2020
4badc14
Merge branch 'master' into revertAbortController
lukasolson Feb 12, 2020
c9352e3
Merge branch 'master' into revertAbortController
lukasolson Feb 14, 2020
100a728
Merge branch 'revertAbortController' into abortSearch
lukasolson Feb 14, 2020
35b1abb
Fix test to use fake timers to run debounced handlers
lukasolson Feb 14, 2020
0db590e
Merge branch 'master' into abortSearch
lukasolson Feb 18, 2020
6cff829
Merge branch 'master' into abortSearch
lukasolson Feb 18, 2020
914d330
Fix loading bar not going away when cancelling
lukasolson Feb 18, 2020
fb5ef63
Merge branch 'master' into abortSearch
lukasolson Feb 18, 2020
c8c1a95
Merge branch 'master' into abortSearch
lukasolson Feb 19, 2020
365ace1
Merge branch 'master' into abortSearch
elasticmachine Feb 20, 2020
f0c2c9c
Merge branch 'master' into abortSearch
elasticmachine Feb 24, 2020
159f5f0
Add test for loading count
lukasolson Feb 24, 2020
5bbae56
Merge remote-tracking branch 'origin/abortSearch' into abortSearch
lukasolson Feb 24, 2020
7f55c43
Merge branch 'master' into abortSearch
lukasolson Feb 25, 2020
efa6cf6
Fix test
lukasolson Feb 25, 2020
2ec0d0b
Fix failing test
lukasolson Feb 25, 2020
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
2 changes: 1 addition & 1 deletion src/plugins/data/public/search/sync_search_strategy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ export const syncSearchStrategyProvider: TSearchStrategyProvider<typeof SYNC_SEA
signal: options.signal,
});

response.then(() => loadingCount$.next(loadingCount$.getValue() - 1));
response.finally(() => loadingCount$.next(loadingCount$.getValue() - 1));
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious, if this is a bug fix and if there should be a test case for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it is and yes there should be.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added in 159f5f0.


return from(response);
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import { IRouter } from 'kibana/server';

import { IFieldType, Filter } from '../index';
import { findIndexPatternById, getFieldByName } from '../index_patterns';
import { getRequestAbortedSignal } from '../lib';

export function registerValueSuggestionsRoute(router: IRouter) {
router.post(
Expand Down Expand Up @@ -50,6 +51,7 @@ export function registerValueSuggestionsRoute(router: IRouter) {
const { field: fieldName, query, boolFilter } = request.body;
const { index } = request.params;
const { dataClient } = context.core.elasticsearch;
const signal = getRequestAbortedSignal(request.events.aborted$);

const autocompleteSearchOptions = {
timeout: await uiSettings.get<number>('kibana.autocompleteTimeout'),
Expand All @@ -62,7 +64,7 @@ export function registerValueSuggestionsRoute(router: IRouter) {
const body = await getBody(autocompleteSearchOptions, field || fieldName, query, boolFilter);

try {
const result = await dataClient.callAsCurrentUser('search', { index, body });
const result = await dataClient.callAsCurrentUser('search', { index, body }, { signal });

const buckets: any[] =
get(result, 'aggregations.suggestions.buckets') ||
Expand Down
45 changes: 45 additions & 0 deletions src/plugins/data/server/lib/get_request_aborted_signal.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import { Subject } from 'rxjs';
import { getRequestAbortedSignal } from './get_request_aborted_signal';

describe('abortableRequestHandler', () => {
jest.useFakeTimers();

it('should call abort if disconnected', () => {
const abortedSubject = new Subject<void>();
const aborted$ = abortedSubject.asObservable();
const onAborted = jest.fn();

const signal = getRequestAbortedSignal(aborted$);
signal.addEventListener('abort', onAborted);

// Shouldn't be aborted or call onAborted prior to disconnecting
expect(signal.aborted).toBe(false);
expect(onAborted).not.toBeCalled();

abortedSubject.next();
jest.runAllTimers();

// Should be aborted and call onAborted after disconnecting
expect(signal.aborted).toBe(true);
expect(onAborted).toBeCalled();
});
});
33 changes: 33 additions & 0 deletions src/plugins/data/server/lib/get_request_aborted_signal.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import { Observable } from 'rxjs';
// @ts-ignore not typed
import { AbortController } from 'abortcontroller-polyfill/dist/cjs-ponyfill';

/**
* A simple utility function that returns an `AbortSignal` corresponding to an `AbortController`
* which aborts when the given request is aborted.
* @param aborted$ The observable of abort events (usually `request.events.aborted$`)
*/
export function getRequestAbortedSignal(aborted$: Observable<void>): AbortSignal {
const controller = new AbortController();
aborted$.subscribe(() => controller.abort());
return controller.signal;
}
20 changes: 20 additions & 0 deletions src/plugins/data/server/lib/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

export { getRequestAbortedSignal } from './get_request_aborted_signal';
5 changes: 4 additions & 1 deletion src/plugins/data/server/search/routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

import { schema } from '@kbn/config-schema';
import { IRouter } from '../../../../core/server';
import { getRequestAbortedSignal } from '../lib';

export function registerSearchRoute(router: IRouter): void {
router.post(
Expand All @@ -35,8 +36,10 @@ export function registerSearchRoute(router: IRouter): void {
async (context, request, res) => {
const searchRequest = request.body;
const strategy = request.params.strategy;
const signal = getRequestAbortedSignal(request.events.aborted$);

try {
const response = await context.search!.search(searchRequest, {}, strategy);
const response = await context.search!.search(searchRequest, { signal }, strategy);
return res.ok({ body: response });
} catch (err) {
return res.customError({
Expand Down