Skip to content

Commit e4d7e85

Browse files
authored
cancel execution on triggered abort signal despite hanging async resolvers (#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 0ffc1e1 commit e4d7e85

File tree

5 files changed

+409
-56
lines changed

5 files changed

+409
-56
lines changed

src/execution/IncrementalPublisher.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { pathToArray } from '../jsutils/Path.js';
55
import type { GraphQLError } from '../error/GraphQLError.js';
66

77
import { IncrementalGraph } from './IncrementalGraph.js';
8+
import type { PromiseCanceller } from './PromiseCanceller.js';
89
import type {
910
CancellableStreamRecord,
1011
CompletedExecutionGroup,
@@ -43,6 +44,7 @@ export function buildIncrementalResponse(
4344
}
4445

4546
interface IncrementalPublisherContext {
47+
promiseCanceller: PromiseCanceller | undefined;
4648
cancellableStreams: Set<CancellableStreamRecord> | undefined;
4749
}
4850

@@ -125,6 +127,7 @@ class IncrementalPublisher {
125127
IteratorResult<SubsequentIncrementalExecutionResult, void>
126128
> => {
127129
if (isDone) {
130+
this._context.promiseCanceller?.disconnect();
128131
await this._returnAsyncIteratorsIgnoringErrors();
129132
return { value: undefined, done: true };
130133
}
@@ -171,6 +174,9 @@ class IncrementalPublisher {
171174
batch = await this._incrementalGraph.nextCompletedBatch();
172175
} while (batch !== undefined);
173176

177+
// TODO: add test for this case
178+
/* c8 ignore next */
179+
this._context.promiseCanceller?.disconnect();
174180
await this._returnAsyncIteratorsIgnoringErrors();
175181
return { value: undefined, done: true };
176182
};

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+
}
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+
});

0 commit comments

Comments
 (0)