remove unnecessary Executor property (#4547)

onExternalAbort can be kept local

the standardization/refactoring handles artificial edge case where
execution finishes synchronously, but info.abortSignal is retained and
then accessed artificially, requiring slight modification of existing
test

Author: yaacovCR

src/execution/Executor.ts

@@ -377,9 +377,8 @@ export interface StreamItemResult {
 export class Executor {
   validatedExecutionArgs: ValidatedExecutionArgs;
   deferUsageSet?: DeferUsageSet | undefined;
-  onExternalAbort: (() => void) | undefined;
   finished: boolean;
-  abortControllers: Set<AbortController>;
+  resolverAbortControllers: Set<AbortController>;
   collectedErrors: CollectedErrors;
   groups: Array<DeliveryGroup>;
   tasks: Array<ExecutionGroup>;
@@ -403,9 +402,8 @@ export class Executor {
   ) {
     this.validatedExecutionArgs = validatedExecutionArgs;
     this.deferUsageSet = deferUsageSet;
-    this.onExternalAbort = undefined;
     this.finished = false;
-    this.abortControllers = new Set();
+    this.resolverAbortControllers = new Set();
     this.collectedErrors = new CollectedErrors();
     this.groups = [];
     this.tasks = [];
@@ -439,17 +437,22 @@ export class Executor {
   > {
     const validatedExecutionArgs = this.validatedExecutionArgs;
     const externalAbortSignal = validatedExecutionArgs.externalAbortSignal;
+    let removeAbortListener: (() => void) | undefined;
     if (externalAbortSignal) {
       if (externalAbortSignal.aborted) {
         throw new Error(externalAbortSignal.reason);
       }
-      const onExternalAbort = () => {
-        this.cancel(externalAbortSignal.reason);
-      };
+      const onExternalAbort = () => this.cancel(externalAbortSignal.reason);
+      removeAbortListener = () =>
+        externalAbortSignal.removeEventListener('abort', onExternalAbort);
       externalAbortSignal.addEventListener('abort', onExternalAbort);
-      this.onExternalAbort = onExternalAbort;
     }
 
+    const onFinish = () => {
+      removeAbortListener?.();
+      this.finish();
+    };
+
     try {
       const {
         schema,
@@ -490,11 +493,11 @@ export class Executor {
       if (isPromise(result)) {
         const promise = result.then(
           (data) => {
-            this.finish();
+            onFinish();
             return this.buildResponse(data);
           },
           (error: unknown) => {
-            this.finish();
+            onFinish();
             this.collectedErrors.add(error as GraphQLError, undefined);
             return this.buildResponse(null);
           },
@@ -503,9 +506,11 @@ export class Executor {
           ? cancellablePromise(promise, externalAbortSignal)
           : promise;
       }
+      onFinish();
       return this.buildResponse(result);
     } catch (error) {
       this.collectedErrors.add(error as GraphQLError, undefined);
+      onFinish();
       return this.buildResponse(null);
     }
   }
@@ -523,24 +528,20 @@ export class Executor {
   }
 
   finish(reason?: unknown): void {
-    if (this.finished) {
-      return;
+    if (!this.finished) {
+      this.finished = true;
+      this.triggerResolverAbortSignals(reason);
     }
-    this.finished = true;
-    const { abortControllers, onExternalAbort } = this;
+  }
+
+  triggerResolverAbortSignals(reason?: unknown): void {
+    const { resolverAbortControllers } = this;
     const finishReason =
       reason ?? new Error('Execution has already completed.');
-    for (const abortController of abortControllers) {
+    for (const abortController of resolverAbortControllers) {
       abortController.abort(finishReason);
     }
-    if (onExternalAbort) {
-      this.validatedExecutionArgs.externalAbortSignal?.removeEventListener(
-        'abort',
-        onExternalAbort,
-      );
-    }
   }
-
   /**
    * Given a completed execution context and data, build the `{ errors, data }`
    * response defined by the "Response" section of the GraphQL specification.
@@ -788,11 +789,11 @@ export class Executor {
           throw new Error('Execution has already completed.');
         }
         const abortController = new AbortController();
-        this.abortControllers.add(abortController);
+        this.resolverAbortControllers.add(abortController);
         return {
           abortSignal: abortController.signal,
           unregister: () => {
-            this.abortControllers.delete(abortController);
+            this.resolverAbortControllers.delete(abortController);
           },
         };
       },

src/execution/__tests__/executor-test.ts

@@ -5,6 +5,7 @@ import { expectJSON } from '../../__testUtils__/expectJSON.js';
 import { resolveOnNextTick } from '../../__testUtils__/resolveOnNextTick.js';
 
 import { inspect } from '../../jsutils/inspect.js';
+import { promiseWithResolvers } from '../../jsutils/promiseWithResolvers.js';
 
 import { Kind } from '../../language/kinds.js';
 import { parse } from '../../language/parser.js';
@@ -192,7 +193,9 @@ describe('Execute: Handles basic execution tasks', () => {
     });
   });
 
-  it('provides info about current execution state', () => {
+  it('provides info about current execution state', async () => {
+    // eslint-disable-next-line @typescript-eslint/no-invalid-void-type
+    const { promise, resolve } = promiseWithResolvers<void>();
     let resolvedInfo: GraphQLResolveInfo | undefined;
     const testType = new GraphQLObjectType({
       name: 'Test',
@@ -201,6 +204,7 @@ describe('Execute: Handles basic execution tasks', () => {
           type: GraphQLString,
           resolve(_val, _args, _ctx, info) {
             resolvedInfo = info;
+            return promise;
           },
         },
       },
@@ -211,7 +215,7 @@ describe('Execute: Handles basic execution tasks', () => {
     const rootValue = { root: 'val' };
     const variableValues = { var: 'abc' };
 
-    executeSync({ schema, document, rootValue, variableValues });
+    const result = execute({ schema, document, rootValue, variableValues });
 
     const operation = document.definitions[0];
     assert(operation.kind === Kind.OPERATION_DEFINITION);
@@ -246,6 +250,10 @@ describe('Execute: Handles basic execution tasks', () => {
     });
 
     expect(resolvedInfo.abortSignal).to.be.instanceOf(AbortSignal);
+
+    resolve();
+
+    await result;
   });
 
   it('populates path correctly with complex types', () => {