Revert "abort without a pending next"

This reverts commit b42febb.

Author: yaacovCR

src/execution/PromiseCanceller.ts

@@ -28,14 +28,19 @@ export class PromiseCanceller {
     this.abortSignal.removeEventListener('abort', this.abort);
   }
 
-  cancellablePromise<T>(originalPromise: Promise<T>): Promise<T> {
+  cancellablePromise<T>(
+    originalPromise: Promise<T>,
+    onCancel?: (() => unknown) | undefined,
+  ): Promise<T> {
     if (this.abortSignal.aborted) {
+      onCancel?.();
       // eslint-disable-next-line @typescript-eslint/prefer-promise-reject-errors
       return Promise.reject(this.abortSignal.reason);
     }
 
     const { promise, resolve, reject } = promiseWithResolvers<T>();
     const abort = () => {
+      onCancel?.();
       reject(this.abortSignal.reason);
     };
     this._aborts.add(abort);
@@ -57,45 +62,20 @@ export class PromiseCanceller {
     const iterator = iterable[Symbol.asyncIterator]();
 
     if (iterator.return) {
-      const _next = iterator.next.bind(iterator);
       const _return = iterator.return.bind(iterator);
-
-      const abort = () => {
+      const _returnIgnoringErrors = async (): Promise<IteratorResult<T>> => {
         _return().catch(() => {
           /* c8 ignore next */
           // ignore
         });
+        return Promise.resolve({ value: undefined, done: true });
       };
 
-      if (this.abortSignal.aborted) {
-        abort();
-        // eslint-disable-next-line @typescript-eslint/prefer-promise-reject-errors
-        const onMethod = () => Promise.reject(this.abortSignal.reason);
-        return {
-          [Symbol.asyncIterator]: () => ({
-            next: onMethod,
-            return: onMethod,
-          }),
-        };
-      }
-
-      this._aborts.add(abort);
-      const on = (method: () => Promise<IteratorResult<T>>) => async () => {
-        try {
-          const iteration = await this.cancellablePromise(method());
-          if (iteration.done) {
-            this._aborts.delete(abort);
-          }
-          return iteration;
-        } catch (error) {
-          this._aborts.delete(abort);
-          throw error;
-        }
-      };
       return {
         [Symbol.asyncIterator]: () => ({
-          next: on(_next),
-          return: on(_return),
+          next: () =>
+            this.cancellablePromise(iterator.next(), _returnIgnoringErrors),
+          return: () => this.cancellablePromise(_return()),
         }),
       };
     }

src/execution/__tests__/PromiseCanceller-test.ts

@@ -70,6 +70,62 @@ describe('PromiseCanceller', () => {
 
       await expectPromise(withCancellation).toRejectWith('Cancelled!');
     });
+
+    it('works to trigger onCancel when cancelling a hanging promise', async () => {
+      const abortController = new AbortController();
+      const abortSignal = abortController.signal;
+
+      const promiseCanceller = new PromiseCanceller(abortSignal);
+
+      const promise = new Promise(() => {
+        /* never resolves */
+      });
+
+      let onCancelCalled = false;
+      const onCancel = () => {
+        onCancelCalled = true;
+      };
+
+      const withCancellation = promiseCanceller.cancellablePromise(
+        promise,
+        onCancel,
+      );
+
+      expect(onCancelCalled).to.equal(false);
+
+      abortController.abort(new Error('Cancelled!'));
+
+      expect(onCancelCalled).to.equal(true);
+
+      await expectPromise(withCancellation).toRejectWith('Cancelled!');
+    });
+
+    it('works to trigger onCancel when cancelling 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 */
+      });
+
+      let onCancelCalled = false;
+      const onCancel = () => {
+        onCancelCalled = true;
+      };
+
+      const withCancellation = promiseCanceller.cancellablePromise(
+        promise,
+        onCancel,
+      );
+
+      expect(onCancelCalled).to.equal(true);
+
+      await expectPromise(withCancellation).toRejectWith('Cancelled!');
+    });
   });
 
   describe('cancellableAsyncIterable', () => {
@@ -141,11 +197,13 @@ describe('PromiseCanceller', () => {
 
       abortController.abort(new Error('Cancelled!'));
 
-      expect(returned).to.equal(true);
+      expect(returned).to.equal(false);
 
       const nextPromise =
         cancellableAsyncIterable[Symbol.asyncIterator]().next();
 
+      expect(returned).to.equal(true);
+
       await expectPromise(nextPromise).toRejectWith('Cancelled!');
     });
 
@@ -160,8 +218,7 @@ describe('PromiseCanceller', () => {
       let returned = false;
       const asyncIterable = {
         [Symbol.asyncIterator]: () => ({
-          /* c8 ignore next */
-          next: () => Promise.resolve({ value: undefined, done: true }),
+          next: () => Promise.resolve({ value: 1, done: false }),
           return: () => {
             returned = true;
             return Promise.resolve({ value: undefined, done: true });
@@ -172,11 +229,13 @@ describe('PromiseCanceller', () => {
       const cancellableAsyncIterable =
         promiseCanceller.cancellableIterable(asyncIterable);
 
-      expect(returned).to.equal(true);
+      expect(returned).to.equal(false);
 
       const nextPromise =
         cancellableAsyncIterable[Symbol.asyncIterator]().next();
 
+      expect(returned).to.equal(true);
+
       await expectPromise(nextPromise).toRejectWith('Cancelled!');
     });
   });

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

@@ -795,7 +795,7 @@ describe('Execute: Cancellation', () => {
     });
   });
 
-  it('should stop the execution when aborted prior to return of the subscription resolver', async () => {
+  it('should stop the execution when aborted during subscription', async () => {
     const abortController = new AbortController();
     const document = parse(`
       subscription {
@@ -830,42 +830,6 @@ describe('Execute: Cancellation', () => {
     });
   });
 
-  it('should successfully wrap the subscription', async () => {
-    const abortController = new AbortController();
-    const document = parse(`
-      subscription {
-        foo
-      }
-    `);
-
-    const subscription = subscribe({
-      document,
-      schema,
-      abortSignal: abortController.signal,
-      rootValue: {
-        async *foo() {
-          yield await Promise.resolve({ foo: 'foo' });
-        },
-      },
-    });
-
-    assert(isAsyncIterable(subscription));
-
-    expectJSON(await subscription.next()).toDeepEqual({
-      value: {
-        data: {
-          foo: 'foo',
-        },
-      },
-      done: false,
-    });
-
-    expectJSON(await subscription.next()).toDeepEqual({
-      value: undefined,
-      done: true,
-    });
-  });
-
   it('should stop the execution when aborted during subscription', async () => {
     const abortController = new AbortController();
     const document = parse(`
@@ -880,6 +844,7 @@ describe('Execute: Cancellation', () => {
       abortSignal: abortController.signal,
       rootValue: {
         async *foo() {
+          yield await Promise.resolve({ foo: 'foo' });
           yield await Promise.resolve({ foo: 'foo' }); /* c8 ignore start */
         } /* c8 ignore stop */,
       },