Skip to content

Commit e94d349

Browse files
DosantDosant
committed
feedback
1 parent 22c09c7 commit e94d349

6 files changed

Lines changed: 79 additions & 28 deletions

File tree

‎src/plugins/data/common/utils/abort_utils.test.ts‎

Lines changed: 60 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
* under the License.
1818
*/
1919

20-
import { AbortError, toPromise, getCombinedController } from './abort_utils';
20+
import { AbortError, toPromise, getCombinedSignal } from './abort_utils';
2121

2222
jest.useFakeTimers();
2323

@@ -41,7 +41,7 @@ describe('AbortUtils', () => {
4141
describe('rejects', () => {
4242
test('should not reject if the signal does not abort', async () => {
4343
const controller = new AbortController();
44-
const promise = toPromise(controller.signal);
44+
const promise = toPromise(controller.signal).promise;
4545
const whenRejected = jest.fn();
4646
promise.catch(whenRejected);
4747
await flushPromises();
@@ -50,7 +50,7 @@ describe('AbortUtils', () => {
5050

5151
test('should reject if the signal does abort', async () => {
5252
const controller = new AbortController();
53-
const promise = toPromise(controller.signal);
53+
const promise = toPromise(controller.signal).promise;
5454
const whenRejected = jest.fn();
5555
promise.catch(whenRejected);
5656
controller.abort();
@@ -67,29 +67,29 @@ describe('AbortUtils', () => {
6767

6868
test('calling clean up handler prevents rejects', async () => {
6969
const controller = new AbortController();
70-
const promise = toPromise(controller.signal);
70+
const { promise, cleanup } = toPromise(controller.signal);
7171
const whenRejected = jest.fn();
7272
promise.catch(whenRejected);
73-
promise.cleanup();
73+
cleanup();
7474
controller.abort();
7575
await flushPromises();
7676
expect(whenRejected).not.toBeCalled();
7777
});
7878
});
7979
});
8080

81-
describe('getCombinedController', () => {
82-
test('should return an AbortController', () => {
83-
const controller = getCombinedController([]);
84-
expect(controller).toBeInstanceOf(AbortController);
81+
describe('getCombinedSignal', () => {
82+
test('should return an AbortSignal', () => {
83+
const signal = getCombinedSignal([]).signal;
84+
expect(signal).toBeInstanceOf(AbortSignal);
8585
});
8686

8787
test('should not abort if none of the signals abort', async () => {
8888
const controller1 = new AbortController();
8989
const controller2 = new AbortController();
9090
setTimeout(() => controller1.abort(), 2000);
9191
setTimeout(() => controller2.abort(), 1000);
92-
const signal = getCombinedController([controller1.signal, controller2.signal]).signal;
92+
const signal = getCombinedSignal([controller1.signal, controller2.signal]).signal;
9393
expect(signal.aborted).toBe(false);
9494
jest.advanceTimersByTime(500);
9595
await flushPromises();
@@ -101,7 +101,7 @@ describe('AbortUtils', () => {
101101
const controller2 = new AbortController();
102102
setTimeout(() => controller1.abort(), 2000);
103103
setTimeout(() => controller2.abort(), 1000);
104-
const signal = getCombinedController([controller1.signal, controller2.signal]).signal;
104+
const signal = getCombinedSignal([controller1.signal, controller2.signal]).signal;
105105
expect(signal.aborted).toBe(false);
106106
jest.advanceTimersByTime(1000);
107107
await flushPromises();
@@ -112,8 +112,56 @@ describe('AbortUtils', () => {
112112
const controller1 = new AbortController();
113113
const controller2 = new AbortController();
114114
controller1.abort();
115-
const signal = getCombinedController([controller1.signal, controller2.signal]).signal;
115+
const signal = getCombinedSignal([controller1.signal, controller2.signal]).signal;
116116
expect(signal.aborted).toBe(true);
117117
});
118+
119+
describe('cleanup listener', () => {
120+
const createMockController = () => {
121+
const controller = new AbortController();
122+
const spyAddListener = jest.spyOn(controller.signal, 'addEventListener');
123+
const spyRemoveListener = jest.spyOn(controller.signal, 'removeEventListener');
124+
return {
125+
controller,
126+
getTotalListeners: () =>
127+
Math.max(spyAddListener.mock.calls.length - spyRemoveListener.mock.calls.length, 0),
128+
};
129+
};
130+
131+
test('cleanup should cleanup inner listeners', () => {
132+
const controller1 = createMockController();
133+
const controller2 = createMockController();
134+
135+
const { cleanup } = getCombinedSignal([
136+
controller1.controller.signal,
137+
controller2.controller.signal,
138+
]);
139+
140+
expect(controller1.getTotalListeners()).toBe(1);
141+
expect(controller2.getTotalListeners()).toBe(1);
142+
143+
cleanup();
144+
145+
expect(controller1.getTotalListeners()).toBe(0);
146+
expect(controller2.getTotalListeners()).toBe(0);
147+
});
148+
149+
test('abort should cleanup inner listeners', async () => {
150+
const controller1 = createMockController();
151+
const controller2 = createMockController();
152+
153+
getCombinedSignal([controller1.controller.signal, controller2.controller.signal]);
154+
155+
expect(controller1.getTotalListeners()).toBe(1);
156+
expect(controller2.getTotalListeners()).toBe(1);
157+
158+
controller1.controller.abort();
159+
160+
await flushPromises();
161+
162+
expect(controller1.getTotalListeners()).toBe(0);
163+
expect(controller2.getTotalListeners()).toBe(0);
164+
});
165+
});
118166
});
119167
});

‎src/plugins/data/common/utils/abort_utils.ts‎

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ export class AbortError extends Error {
3636
*
3737
* @param signal The `AbortSignal` to generate the `Promise` from
3838
*/
39-
export function toPromise(signal: AbortSignal): Promise<never> & { cleanup: () => void } {
39+
export function toPromise(signal: AbortSignal): { promise: Promise<never>; cleanup: () => void } {
4040
let abortHandler: () => void;
4141
const cleanup = () => {
4242
if (abortHandler) {
@@ -52,31 +52,34 @@ export function toPromise(signal: AbortSignal): Promise<never> & { cleanup: () =
5252
signal.addEventListener('abort', abortHandler);
5353
});
5454

55-
return Object.assign(promise, { cleanup });
55+
return { promise, cleanup };
5656
}
5757

5858
/**
59-
* Returns an `AbortController` that will be aborted when the first of the given signals aborts.
59+
* Returns an `AbortSignal` that will be aborted when the first of the given signals aborts.
6060
*
6161
* @param signals
6262
*/
63-
export function getCombinedController(signals: AbortSignal[]) {
63+
export function getCombinedSignal(
64+
signals: AbortSignal[]
65+
): { signal: AbortSignal; cleanup: () => void } {
6466
const controller = new AbortController();
67+
let cleanup = () => {};
6568

6669
if (signals.some((signal) => signal.aborted)) {
6770
controller.abort();
6871
} else {
6972
const promises = signals.map((signal) => toPromise(signal));
70-
const cleanup = () => {
73+
cleanup = () => {
7174
promises.forEach((p) => p.cleanup());
7275
controller.signal.removeEventListener('abort', cleanup);
7376
};
7477
controller.signal.addEventListener('abort', cleanup);
75-
Promise.race(promises).catch(() => {
78+
Promise.race(promises.map((p) => p.promise)).catch(() => {
7679
cleanup();
7780
controller.abort();
7881
});
7982
}
8083

81-
return controller;
84+
return { signal: controller.signal, cleanup };
8285
}

‎src/plugins/data/common/utils/index.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,4 +19,4 @@
1919

2020
/** @internal */
2121
export { shortenDottedString } from './shorten_dotted_string';
22-
export { AbortError, toPromise, getCombinedController } from './abort_utils';
22+
export { AbortError, toPromise, getCombinedSignal } from './abort_utils';

‎src/plugins/data/public/search/search_interceptor.ts‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ import {
3030
ISearchOptions,
3131
ES_SEARCH_STRATEGY,
3232
ISessionService,
33-
getCombinedController,
33+
getCombinedSignal,
3434
} from '../../common';
3535
import { SearchUsageCollector } from './collectors';
3636
import {
@@ -171,17 +171,17 @@ export class SearchInterceptor {
171171
...(abortSignal ? [abortSignal] : []),
172172
];
173173

174-
const combinedController = getCombinedController(signals);
174+
const { signal: combinedSignal, cleanup: cleanupCombinedSignal } = getCombinedSignal(signals);
175175
const cleanup = () => {
176176
subscription.unsubscribe();
177-
combinedController.signal.removeEventListener('abort', cleanup);
178-
combinedController.abort();
177+
combinedSignal.removeEventListener('abort', cleanup);
178+
cleanupCombinedSignal();
179179
};
180-
combinedController.signal.addEventListener('abort', cleanup);
180+
combinedSignal.addEventListener('abort', cleanup);
181181

182182
return {
183183
timeoutSignal,
184-
combinedSignal: combinedController.signal,
184+
combinedSignal,
185185
cleanup,
186186
};
187187
}

‎src/plugins/expressions/common/execution/execution.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ export class Execution<
9999
* Races a given promise against the "abort" event of `abortController`.
100100
*/
101101
private race<T>(promise: Promise<T>): Promise<T> {
102-
return Promise.race<T>([this.abortRejection, promise]);
102+
return Promise.race<T>([this.abortRejection.promise, promise]);
103103
}
104104

105105
/**

‎x-pack/plugins/data_enhanced/public/search/search_interceptor.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ export class EnhancedSearchInterceptor extends SearchInterceptor {
9090
})
9191
);
9292
}),
93-
takeUntil(from(abortedPromise)),
93+
takeUntil(from(abortedPromise.promise)),
9494
catchError((e: any) => {
9595
// If we haven't received the response to the initial request, including the ID, then
9696
// we don't need to send a follow-up request to delete this search. Otherwise, we

0 commit comments

Comments
 (0)