feedback and additional tests

Author: yaacovCR

src/execution/IncrementalPublisher.ts

@@ -4,8 +4,8 @@ import { pathToArray } from '../jsutils/Path.js';
 
 import type { GraphQLError } from '../error/GraphQLError.js';
 
-import type { Canceller } from './Canceller.js';
 import { IncrementalGraph } from './IncrementalGraph.js';
+import type { PromiseCanceller } from './PromiseCanceller.js';
 import type {
   CancellableStreamRecord,
   CompletedExecutionGroup,
@@ -44,7 +44,7 @@ export function buildIncrementalResponse(
 }
 
 interface IncrementalPublisherContext {
-  canceller: Canceller | undefined;
+  promiseCanceller: PromiseCanceller | undefined;
   cancellableStreams: Set<CancellableStreamRecord> | undefined;
 }
 
@@ -127,7 +127,7 @@ class IncrementalPublisher {
       IteratorResult<SubsequentIncrementalExecutionResult, void>
     > => {
       if (isDone) {
-        this._context.canceller?.unsubscribe();
+        this._context.promiseCanceller?.disconnect();
         await this._returnAsyncIteratorsIgnoringErrors();
         return { value: undefined, done: true };
       }
@@ -176,7 +176,7 @@ class IncrementalPublisher {
 
       // TODO: add test for this case
       /* c8 ignore next */
-      this._context.canceller?.unsubscribe();
+      this._context.promiseCanceller?.disconnect();
       await this._returnAsyncIteratorsIgnoringErrors();
       return { value: undefined, done: true };
     };

src/execution/Canceller.ts renamed to src/execution/PromiseCanceller.ts

@@ -1,12 +1,12 @@
 import { promiseWithResolvers } from '../jsutils/promiseWithResolvers.js';
 
 /**
- * A Canceller object that can be used to cancel multiple promises
+ * A PromiseCanceller object can be used to cancel multiple promises
  * using a single AbortSignal.
  *
  * @internal
  */
-export class Canceller {
+export class PromiseCanceller {
   abortSignal: AbortSignal;
   abort: () => void;
 
@@ -24,11 +24,16 @@ export class Canceller {
     abortSignal.addEventListener('abort', this.abort);
   }
 
-  unsubscribe(): void {
+  disconnect(): void {
     this.abortSignal.removeEventListener('abort', this.abort);
   }
 
   withCancellation<T>(originalPromise: Promise<T>): Promise<T> {
+    if (this.abortSignal.aborted) {
+      // eslint-disable-next-line @typescript-eslint/prefer-promise-reject-errors
+      return Promise.reject(this.abortSignal.reason);
+    }
+
     const { promise, resolve, reject } = promiseWithResolvers<T>();
     const abort = () => reject(this.abortSignal.reason);
     this._aborts.add(abort);

src/execution/__tests__/PromiseCanceller-test.ts

@@ -0,0 +1,56 @@
+import { describe, it } from 'mocha';
+
+import { expectPromise } from '../../__testUtils__/expectPromise.js';
+
+import { PromiseCanceller } from '../PromiseCanceller.js';
+
+describe('PromiseCanceller', () => {
+  it('works to cancel an already resolved promise', async () => {
+    const abortController = new AbortController();
+    const abortSignal = abortController.signal;
+
+    const promiseCanceller = new PromiseCanceller(abortSignal);
+
+    const promise = Promise.resolve(1);
+
+    const withCancellation = promiseCanceller.withCancellation(promise);
+
+    abortController.abort(new Error('Cancelled!'));
+
+    await expectPromise(withCancellation).toRejectWith('Cancelled!');
+  });
+
+  it('works to cancel a hanging promise', async () => {
+    const abortController = new AbortController();
+    const abortSignal = abortController.signal;
+
+    const promiseCanceller = new PromiseCanceller(abortSignal);
+
+    const promise = new Promise(() => {
+      /* never resolves */
+    });
+
+    const withCancellation = promiseCanceller.withCancellation(promise);
+
+    abortController.abort(new Error('Cancelled!'));
+
+    await expectPromise(withCancellation).toRejectWith('Cancelled!');
+  });
+
+  it('works to cancel a hanging promise created after abort signal triggered', async () => {
+    const abortController = new AbortController();
+    const abortSignal = abortController.signal;
+
+    abortController.abort(new Error('Cancelled!'));
+
+    const promiseCanceller = new PromiseCanceller(abortSignal);
+
+    const promise = new Promise(() => {
+      /* never resolves */
+    });
+
+    const withCancellation = promiseCanceller.withCancellation(promise);
+
+    await expectPromise(withCancellation).toRejectWith('Cancelled!');
+  });
+});

src/execution/execute.ts

@@ -50,7 +50,6 @@ import { assertValidSchema } from '../type/validate.js';
 
 import type { DeferUsageSet, ExecutionPlan } from './buildExecutionPlan.js';
 import { buildExecutionPlan } from './buildExecutionPlan.js';
-import { Canceller } from './Canceller.js';
 import type {
   DeferUsage,
   FieldDetailsList,
@@ -64,6 +63,7 @@ import {
 import { getVariableSignature } from './getVariableSignature.js';
 import { buildIncrementalResponse } from './IncrementalPublisher.js';
 import { mapAsyncIterable } from './mapAsyncIterable.js';
+import { PromiseCanceller } from './PromiseCanceller.js';
 import type {
   CancellableStreamRecord,
   CompletedExecutionGroup,
@@ -164,7 +164,7 @@ export interface ValidatedExecutionArgs {
 export interface ExecutionContext {
   validatedExecutionArgs: ValidatedExecutionArgs;
   errors: Array<GraphQLError> | undefined;
-  canceller: Canceller | undefined;
+  promiseCanceller: PromiseCanceller | undefined;
   cancellableStreams: Set<CancellableStreamRecord> | undefined;
 }
 
@@ -316,7 +316,9 @@ export function experimentalExecuteQueryOrMutationOrSubscriptionEvent(
   const exeContext: ExecutionContext = {
     validatedExecutionArgs,
     errors: undefined,
-    canceller: abortSignal ? new Canceller(abortSignal) : undefined,
+    promiseCanceller: abortSignal
+      ? new PromiseCanceller(abortSignal)
+      : undefined,
     cancellableStreams: undefined,
   };
   try {
@@ -369,7 +371,7 @@ export function experimentalExecuteQueryOrMutationOrSubscriptionEvent(
       return graphqlWrappedResult.then(
         (resolved) => buildDataResponse(exeContext, resolved),
         (error: unknown) => {
-          exeContext.canceller?.unsubscribe();
+          exeContext.promiseCanceller?.disconnect();
           return {
             data: null,
             errors: withError(exeContext.errors, error as GraphQLError),
@@ -381,7 +383,7 @@ export function experimentalExecuteQueryOrMutationOrSubscriptionEvent(
   } catch (error) {
     // TODO: add test case for synchronous null bubbling to root with cancellation
     /* c8 ignore next */
-    exeContext.canceller?.unsubscribe();
+    exeContext.promiseCanceller?.disconnect();
     return { data: null, errors: withError(exeContext.errors, error) };
   }
 }
@@ -472,7 +474,7 @@ function buildDataResponse(
   const { rawResult: data, incrementalDataRecords } = graphqlWrappedResult;
   const errors = exeContext.errors;
   if (incrementalDataRecords === undefined) {
-    exeContext.canceller?.unsubscribe();
+    exeContext.promiseCanceller?.disconnect();
     return errors !== undefined ? { errors, data } : { data };
   }
 
@@ -823,7 +825,7 @@ function executeField(
   incrementalContext: IncrementalContext | undefined,
   deferMap: ReadonlyMap<DeferUsage, DeferredFragmentRecord> | undefined,
 ): PromiseOrValue<GraphQLWrappedResult<unknown>> | undefined {
-  const { validatedExecutionArgs, canceller } = exeContext;
+  const { validatedExecutionArgs, promiseCanceller } = exeContext;
   const { schema, contextValue, variableValues, hideSuggestions, abortSignal } =
     validatedExecutionArgs;
   const fieldName = fieldDetailsList[0].node.name.value;
@@ -868,7 +870,7 @@ function executeField(
         fieldDetailsList,
         info,
         path,
-        canceller?.withCancellation(result) ?? result,
+        promiseCanceller?.withCancellation(result) ?? result,
         incrementalContext,
         deferMap,
       );
@@ -2203,17 +2205,19 @@ function executeSubscription(
     const result = resolveFn(rootValue, args, contextValue, info, abortSignal);
 
     if (isPromise(result)) {
-      const canceller = abortSignal ? new Canceller(abortSignal) : undefined;
-      const promise = canceller?.withCancellation(result) ?? result;
+      const promiseCanceller = abortSignal
+        ? new PromiseCanceller(abortSignal)
+        : undefined;
+      const promise = promiseCanceller?.withCancellation(result) ?? result;
       return promise.then(assertEventStream).then(
         (resolved) => {
           // TODO: add test case
           /* c8 ignore next */
-          canceller?.unsubscribe();
+          promiseCanceller?.disconnect();
           return resolved;
         },
         (error: unknown) => {
-          canceller?.unsubscribe();
+          promiseCanceller?.disconnect();
           throw locatedError(error, fieldNodes, pathToArray(path));
         },
       );