From 0f49d140a644cbee5ce88bfa07991ab38c86ba18 Mon Sep 17 00:00:00 2001 From: Lukas Olson Date: Tue, 25 Feb 2020 13:51:02 -0700 Subject: [PATCH 1/2] Abort cancelled search requests to Elasticsearch (#56788) * Update abort controller library * Bootstrap * Abort when the request is aborted * Add utility and update value suggestions route * Remove bad merge * Revert switching abort controller libraries * Revert package.json in lib * Move to previous abort controller * Fix test to use fake timers to run debounced handlers * Fix loading bar not going away when cancelling * Add test for loading count * Fix test * Fix failing test Co-authored-by: Elastic Machine --- .../search/sync_search_strategy.test.ts | 43 ++++++++++++++++++ .../autocomplete/value_suggestions_route.ts | 4 +- .../lib/get_request_aborted_signal.test.ts | 45 +++++++++++++++++++ .../server/lib/get_request_aborted_signal.ts | 33 ++++++++++++++ src/plugins/data/server/lib/index.ts | 20 +++++++++ src/plugins/data/server/search/routes.ts | 5 ++- 6 files changed, 148 insertions(+), 2 deletions(-) create mode 100644 src/plugins/data/server/lib/get_request_aborted_signal.test.ts create mode 100644 src/plugins/data/server/lib/get_request_aborted_signal.ts create mode 100644 src/plugins/data/server/lib/index.ts diff --git a/src/plugins/data/public/search/sync_search_strategy.test.ts b/src/plugins/data/public/search/sync_search_strategy.test.ts index 2737a4033a015..e5aa3ba308e58 100644 --- a/src/plugins/data/public/search/sync_search_strategy.test.ts +++ b/src/plugins/data/public/search/sync_search_strategy.test.ts @@ -55,4 +55,47 @@ describe('Sync search strategy', () => { signal: undefined, }); }); + + it('increments and decrements loading count on success', async () => { + const expectedLoadingCountValues = [0, 1, 0]; + const receivedLoadingCountValues: number[] = []; + + mockCoreStart.http.fetch.mockResolvedValueOnce('response'); + + const syncSearch = syncSearchStrategyProvider({ + core: mockCoreStart, + getSearchStrategy: jest.fn(), + }); + const request = { serverStrategy: SYNC_SEARCH_STRATEGY }; + + const loadingCount$ = mockCoreStart.http.addLoadingCountSource.mock.calls[0][0]; + loadingCount$.subscribe(value => receivedLoadingCountValues.push(value)); + + await syncSearch.search(request, {}).toPromise(); + + expect(receivedLoadingCountValues).toEqual(expectedLoadingCountValues); + }); + + it('increments and decrements loading count on failure', async () => { + expect.assertions(1); + const expectedLoadingCountValues = [0, 1, 0]; + const receivedLoadingCountValues: number[] = []; + + mockCoreStart.http.fetch.mockRejectedValueOnce('error'); + + const syncSearch = syncSearchStrategyProvider({ + core: mockCoreStart, + getSearchStrategy: jest.fn(), + }); + const request = { serverStrategy: SYNC_SEARCH_STRATEGY }; + + const loadingCount$ = mockCoreStart.http.addLoadingCountSource.mock.calls[0][0]; + loadingCount$.subscribe(value => receivedLoadingCountValues.push(value)); + + try { + await syncSearch.search(request, {}).toPromise(); + } catch (e) { + expect(receivedLoadingCountValues).toEqual(expectedLoadingCountValues); + } + }); }); diff --git a/src/plugins/data/server/autocomplete/value_suggestions_route.ts b/src/plugins/data/server/autocomplete/value_suggestions_route.ts index e5b07087fd599..6c78b42999456 100644 --- a/src/plugins/data/server/autocomplete/value_suggestions_route.ts +++ b/src/plugins/data/server/autocomplete/value_suggestions_route.ts @@ -24,6 +24,7 @@ import { IRouter, SharedGlobalConfig } from 'kibana/server'; import { Observable } from 'rxjs'; import { first } from 'rxjs/operators'; import { IFieldType, indexPatterns, esFilters } from '../index'; +import { getRequestAbortedSignal } from '../lib'; export function registerValueSuggestionsRoute( router: IRouter, @@ -54,6 +55,7 @@ export function registerValueSuggestionsRoute( 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: `${config.kibana.autocompleteTimeout.asMilliseconds()}ms`, @@ -69,7 +71,7 @@ export function registerValueSuggestionsRoute( 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') || diff --git a/src/plugins/data/server/lib/get_request_aborted_signal.test.ts b/src/plugins/data/server/lib/get_request_aborted_signal.test.ts new file mode 100644 index 0000000000000..3c1e20dbcb158 --- /dev/null +++ b/src/plugins/data/server/lib/get_request_aborted_signal.test.ts @@ -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(); + 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(); + }); +}); diff --git a/src/plugins/data/server/lib/get_request_aborted_signal.ts b/src/plugins/data/server/lib/get_request_aborted_signal.ts new file mode 100644 index 0000000000000..d1541f1df9384 --- /dev/null +++ b/src/plugins/data/server/lib/get_request_aborted_signal.ts @@ -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): AbortSignal { + const controller = new AbortController(); + aborted$.subscribe(() => controller.abort()); + return controller.signal; +} diff --git a/src/plugins/data/server/lib/index.ts b/src/plugins/data/server/lib/index.ts new file mode 100644 index 0000000000000..a2af456846e14 --- /dev/null +++ b/src/plugins/data/server/lib/index.ts @@ -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'; diff --git a/src/plugins/data/server/search/routes.ts b/src/plugins/data/server/search/routes.ts index eaa72548e08ee..37618d14bfc2d 100644 --- a/src/plugins/data/server/search/routes.ts +++ b/src/plugins/data/server/search/routes.ts @@ -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( @@ -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.internalError({ body: err }); From 872a628018ea521c15f13cb562684ad4fee976d4 Mon Sep 17 00:00:00 2001 From: Lukas Olson Date: Fri, 28 Feb 2020 11:46:48 -0700 Subject: [PATCH 2/2] Remove unnecessary tests --- .../search/sync_search_strategy.test.ts | 43 ------------------- 1 file changed, 43 deletions(-) diff --git a/src/plugins/data/public/search/sync_search_strategy.test.ts b/src/plugins/data/public/search/sync_search_strategy.test.ts index e5aa3ba308e58..2737a4033a015 100644 --- a/src/plugins/data/public/search/sync_search_strategy.test.ts +++ b/src/plugins/data/public/search/sync_search_strategy.test.ts @@ -55,47 +55,4 @@ describe('Sync search strategy', () => { signal: undefined, }); }); - - it('increments and decrements loading count on success', async () => { - const expectedLoadingCountValues = [0, 1, 0]; - const receivedLoadingCountValues: number[] = []; - - mockCoreStart.http.fetch.mockResolvedValueOnce('response'); - - const syncSearch = syncSearchStrategyProvider({ - core: mockCoreStart, - getSearchStrategy: jest.fn(), - }); - const request = { serverStrategy: SYNC_SEARCH_STRATEGY }; - - const loadingCount$ = mockCoreStart.http.addLoadingCountSource.mock.calls[0][0]; - loadingCount$.subscribe(value => receivedLoadingCountValues.push(value)); - - await syncSearch.search(request, {}).toPromise(); - - expect(receivedLoadingCountValues).toEqual(expectedLoadingCountValues); - }); - - it('increments and decrements loading count on failure', async () => { - expect.assertions(1); - const expectedLoadingCountValues = [0, 1, 0]; - const receivedLoadingCountValues: number[] = []; - - mockCoreStart.http.fetch.mockRejectedValueOnce('error'); - - const syncSearch = syncSearchStrategyProvider({ - core: mockCoreStart, - getSearchStrategy: jest.fn(), - }); - const request = { serverStrategy: SYNC_SEARCH_STRATEGY }; - - const loadingCount$ = mockCoreStart.http.addLoadingCountSource.mock.calls[0][0]; - loadingCount$.subscribe(value => receivedLoadingCountValues.push(value)); - - try { - await syncSearch.search(request, {}).toPromise(); - } catch (e) { - expect(receivedLoadingCountValues).toEqual(expectedLoadingCountValues); - } - }); });