Skip to content

Commit 2fab868

Browse files
yaacovCRyaacovCR
committed
cancel execution on triggered abort signal despite hanging async resolvers (graphql#4267)
Prior to this pull request, cancellation worked by checking the abort signal status during execution, and throwing the reason if the abort signal has been triggered. This fails if an asynchronous resolver hangs. This pull request changes the cancellation method to wrap promises returned by resolvers so that they immediately reject on cancellation.
1 parent 348a509 commit 2fab868

File tree

4 files changed

+330
-32
lines changed

4 files changed

+330
-32
lines changed

‎src/execution/PromiseCanceller.ts‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
import { promiseWithResolvers } from '../jsutils/promiseWithResolvers.js';
2+
3+
/**
4+
* A PromiseCanceller object can be used to cancel multiple promises
5+
* using a single AbortSignal.
6+
*
7+
* @internal
8+
*/
9+
export class PromiseCanceller {
10+
abortSignal: AbortSignal;
11+
abort: () => void;
12+
13+
private _aborts: Set<() => void>;
14+
15+
constructor(abortSignal: AbortSignal) {
16+
this.abortSignal = abortSignal;
17+
this._aborts = new Set<() => void>();
18+
this.abort = () => {
19+
for (const abort of this._aborts) {
20+
abort();
21+
}
22+
};
23+
24+
abortSignal.addEventListener('abort', this.abort);
25+
}
26+
27+
disconnect(): void {
28+
this.abortSignal.removeEventListener('abort', this.abort);
29+
}
30+
31+
withCancellation<T>(originalPromise: Promise<T>): Promise<T> {
32+
if (this.abortSignal.aborted) {
33+
// eslint-disable-next-line @typescript-eslint/prefer-promise-reject-errors
34+
return Promise.reject(this.abortSignal.reason);
35+
}
36+
37+
const { promise, resolve, reject } = promiseWithResolvers<T>();
38+
const abort = () => reject(this.abortSignal.reason);
39+
this._aborts.add(abort);
40+
originalPromise.then(
41+
(resolved) => {
42+
this._aborts.delete(abort);
43+
resolve(resolved);
44+
},
45+
(error: unknown) => {
46+
this._aborts.delete(abort);
47+
reject(error);
48+
},
49+
);
50+
51+
return promise;
52+
}
53+
}

‎src/execution/__tests__/PromiseCanceller-test.ts‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
import { describe, it } from 'mocha';
2+
3+
import { expectPromise } from '../../__testUtils__/expectPromise.js';
4+
5+
import { PromiseCanceller } from '../PromiseCanceller.js';
6+
7+
describe('PromiseCanceller', () => {
8+
it('works to cancel an already resolved promise', async () => {
9+
const abortController = new AbortController();
10+
const abortSignal = abortController.signal;
11+
12+
const promiseCanceller = new PromiseCanceller(abortSignal);
13+
14+
const promise = Promise.resolve(1);
15+
16+
const withCancellation = promiseCanceller.withCancellation(promise);
17+
18+
abortController.abort(new Error('Cancelled!'));
19+
20+
await expectPromise(withCancellation).toRejectWith('Cancelled!');
21+
});
22+
23+
it('works to cancel a hanging promise', async () => {
24+
const abortController = new AbortController();
25+
const abortSignal = abortController.signal;
26+
27+
const promiseCanceller = new PromiseCanceller(abortSignal);
28+
29+
const promise = new Promise(() => {
30+
/* never resolves */
31+
});
32+
33+
const withCancellation = promiseCanceller.withCancellation(promise);
34+
35+
abortController.abort(new Error('Cancelled!'));
36+
37+
await expectPromise(withCancellation).toRejectWith('Cancelled!');
38+
});
39+
40+
it('works to cancel a hanging promise created after abort signal triggered', async () => {
41+
const abortController = new AbortController();
42+
const abortSignal = abortController.signal;
43+
44+
abortController.abort(new Error('Cancelled!'));
45+
46+
const promiseCanceller = new PromiseCanceller(abortSignal);
47+
48+
const promise = new Promise(() => {
49+
/* never resolves */
50+
});
51+
52+
const withCancellation = promiseCanceller.withCancellation(promise);
53+
54+
await expectPromise(withCancellation).toRejectWith('Cancelled!');
55+
});
56+
});

‎src/execution/__tests__/abort-signal-test.ts‎

Lines changed: 186 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,12 +8,12 @@ import { parse } from '../../language/parser.js';
88

99
import { buildSchema } from '../../utilities/buildASTSchema.js';
1010

11-
import { execute } from '../execute.js';
11+
import { execute, subscribe } from '../execute.js';
1212

1313
const schema = buildSchema(`
1414
type Todo {
1515
id: ID
16-
text: String
16+
items: [String]
1717
author: User
1818
}
1919
@@ -24,12 +24,17 @@ const schema = buildSchema(`
2424
2525
type Query {
2626
todo: Todo
27+
nonNullableTodo: Todo!
2728
}
2829
2930
type Mutation {
3031
foo: String
3132
bar: String
3233
}
34+
35+
type Subscription {
36+
foo: String
37+
}
3338
`);
3439

3540
describe('Execute: Cancellation', () => {
@@ -54,7 +59,6 @@ describe('Execute: Cancellation', () => {
5459
todo: async () =>
5560
Promise.resolve({
5661
id: '1',
57-
text: 'Hello, World!',
5862
/* c8 ignore next */
5963
author: () => expect.fail('Should not be called'),
6064
}),
@@ -149,7 +153,6 @@ describe('Execute: Cancellation', () => {
149153
todo: async () =>
150154
Promise.resolve({
151155
id: '1',
152-
text: 'Hello, World!',
153156
/* c8 ignore next */
154157
author: () => expect.fail('Should not be called'),
155158
}),
@@ -198,7 +201,6 @@ describe('Execute: Cancellation', () => {
198201
todo: async () =>
199202
Promise.resolve({
200203
id: '1',
201-
text: 'Hello, World!',
202204
/* c8 ignore next */
203205
author: () => expect.fail('Should not be called'),
204206
}),
@@ -243,7 +245,6 @@ describe('Execute: Cancellation', () => {
243245
rootValue: {
244246
todo: {
245247
id: '1',
246-
text: 'Hello, World!',
247248
/* c8 ignore next 3 */
248249
author: async () =>
249250
Promise.resolve(() => expect.fail('Should not be called')),
@@ -272,14 +273,153 @@ describe('Execute: Cancellation', () => {
272273
});
273274
});
274275

276+
it('should stop the execution when aborted despite a hanging resolver', async () => {
277+
const abortController = new AbortController();
278+
const document = parse(`
279+
query {
280+
todo {
281+
id
282+
author {
283+
id
284+
}
285+
}
286+
}
287+
`);
288+
289+
const resultPromise = execute({
290+
document,
291+
schema,
292+
abortSignal: abortController.signal,
293+
rootValue: {
294+
todo: () =>
295+
new Promise(() => {
296+
/* will never resolve */
297+
}),
298+
},
299+
});
300+
301+
abortController.abort();
302+
303+
const result = await resultPromise;
304+
305+
expect(result.errors?.[0].originalError?.name).to.equal('AbortError');
306+
307+
expectJSON(result).toDeepEqual({
308+
data: {
309+
todo: null,
310+
},
311+
errors: [
312+
{
313+
message: 'This operation was aborted',
314+
path: ['todo'],
315+
locations: [{ line: 3, column: 9 }],
316+
},
317+
],
318+
});
319+
});
320+
321+
it('should stop the execution when aborted despite a hanging item', async () => {
322+
const abortController = new AbortController();
323+
const document = parse(`
324+
query {
325+
todo {
326+
id
327+
items
328+
}
329+
}
330+
`);
331+
332+
const resultPromise = execute({
333+
document,
334+
schema,
335+
abortSignal: abortController.signal,
336+
rootValue: {
337+
todo: () => ({
338+
id: '1',
339+
items: [
340+
new Promise(() => {
341+
/* will never resolve */
342+
}),
343+
],
344+
}),
345+
},
346+
});
347+
348+
abortController.abort();
349+
350+
const result = await resultPromise;
351+
352+
expect(result.errors?.[0].originalError?.name).to.equal('AbortError');
353+
354+
expectJSON(result).toDeepEqual({
355+
data: {
356+
todo: {
357+
id: '1',
358+
items: [null],
359+
},
360+
},
361+
errors: [
362+
{
363+
message: 'This operation was aborted',
364+
path: ['todo', 'items', 0],
365+
locations: [{ line: 5, column: 11 }],
366+
},
367+
],
368+
});
369+
});
370+
371+
it('should stop the execution when aborted with proper null bubbling', async () => {
372+
const abortController = new AbortController();
373+
const document = parse(`
374+
query {
375+
nonNullableTodo {
376+
id
377+
author {
378+
id
379+
}
380+
}
381+
}
382+
`);
383+
384+
const resultPromise = execute({
385+
document,
386+
schema,
387+
abortSignal: abortController.signal,
388+
rootValue: {
389+
nonNullableTodo: async () =>
390+
Promise.resolve({
391+
id: '1',
392+
/* c8 ignore next */
393+
author: () => expect.fail('Should not be called'),
394+
}),
395+
},
396+
});
397+
398+
abortController.abort();
399+
400+
const result = await resultPromise;
401+
402+
expect(result.errors?.[0].originalError?.name).to.equal('AbortError');
403+
404+
expectJSON(result).toDeepEqual({
405+
data: null,
406+
errors: [
407+
{
408+
message: 'This operation was aborted',
409+
path: ['nonNullableTodo'],
410+
locations: [{ line: 3, column: 9 }],
411+
},
412+
],
413+
});
414+
});
415+
275416
it('should stop deferred execution when aborted', async () => {
276417
const abortController = new AbortController();
277418
const document = parse(`
278419
query {
279420
todo {
280421
id
281422
... on Todo @defer {
282-
text
283423
author {
284424
id
285425
}
@@ -295,7 +435,6 @@ describe('Execute: Cancellation', () => {
295435
todo: async () =>
296436
Promise.resolve({
297437
id: '1',
298-
text: 'hello world',
299438
/* c8 ignore next */
300439
author: () => expect.fail('Should not be called'),
301440
}),
@@ -341,6 +480,10 @@ describe('Execute: Cancellation', () => {
341480
},
342481
});
343482

483+
await resolveOnNextTick();
484+
await resolveOnNextTick();
485+
await resolveOnNextTick();
486+
344487
abortController.abort();
345488

346489
const result = await resultPromise;
@@ -391,4 +534,39 @@ describe('Execute: Cancellation', () => {
391534
],
392535
});
393536
});
537+
538+
it('should stop the execution when aborted during subscription', async () => {
539+
const abortController = new AbortController();
540+
const document = parse(`
541+
subscription {
542+
foo
543+
}
544+
`);
545+
546+
const resultPromise = subscribe({
547+
document,
548+
schema,
549+
abortSignal: abortController.signal,
550+
rootValue: {
551+
foo: async () =>
552+
new Promise(() => {
553+
/* will never resolve */
554+
}),
555+
},
556+
});
557+
558+
abortController.abort();
559+
560+
const result = await resultPromise;
561+
562+
expectJSON(result).toDeepEqual({
563+
errors: [
564+
{
565+
message: 'This operation was aborted',
566+
path: ['foo'],
567+
locations: [{ line: 3, column: 9 }],
568+
},
569+
],
570+
});
571+
});
394572
});

0 commit comments

Comments
 (0)