add abrupt return handling for sync iterables (#4533)

in parallel to handling for async iterables

Author: yaacovCR

src/execution/__tests__/lists-test.ts

@@ -78,6 +78,123 @@ describe('Execute: Accepts any iterable as list value', () => {
   });
 });
 
+describe('Execute: Handles abrupt completion in synchronous iterables', () => {
+  function complete(rootValue: unknown, as: string = '[String]') {
+    return execute({
+      schema: buildSchema(`type Query { listField: ${as} }`),
+      document: parse('{ listField }'),
+      rootValue,
+    });
+  }
+
+  it('closes the iterator when `next` throws', async () => {
+    let returned = false;
+    let nextCalls = 0;
+
+    const listField: IterableIterator<string> = {
+      [Symbol.iterator](): IterableIterator<string> {
+        return this;
+      },
+      next(): IteratorResult<string> {
+        nextCalls++;
+        if (nextCalls === 1) {
+          return { done: false, value: 'ok' };
+        }
+        throw new Error('bad');
+      },
+      return(): IteratorResult<string> {
+        returned = true;
+        return { done: true, value: undefined };
+      },
+    };
+
+    expectJSON(await complete({ listField })).toDeepEqual({
+      data: { listField: null },
+      errors: [
+        {
+          message: 'bad',
+          locations: [{ line: 1, column: 3 }],
+          path: ['listField'],
+        },
+      ],
+    });
+    expect(nextCalls).to.equal(2);
+    expect(returned).to.equal(true);
+  });
+
+  it('closes the iterator when a null bubbles up from a non-null item', async () => {
+    const values = [1, null, 2];
+    let index = 0;
+    let returned = false;
+
+    const listField: IterableIterator<number | null> = {
+      [Symbol.iterator](): IterableIterator<number | null> {
+        return this;
+      },
+      next(): IteratorResult<number | null> {
+        const value = values[index++];
+        if (value === undefined) {
+          return { done: true, value: undefined };
+        }
+        return { done: false, value };
+      },
+      return(): IteratorResult<number | null> {
+        returned = true;
+        return { done: true, value: undefined };
+      },
+    };
+
+    expectJSON(await complete({ listField }, '[Int!]')).toDeepEqual({
+      data: { listField: null },
+      errors: [
+        {
+          message: 'Cannot return null for non-nullable field Query.listField.',
+          locations: [{ line: 1, column: 3 }],
+          path: ['listField', 1],
+        },
+      ],
+    });
+    expect(index).to.equal(2);
+    expect(returned).to.equal(true);
+  });
+
+  it('ignores errors thrown by the iterator `return` method', async () => {
+    const values = [1, null, 2];
+    let index = 0;
+    let returned = false;
+
+    const listField: IterableIterator<number | null> = {
+      [Symbol.iterator](): IterableIterator<number | null> {
+        return this;
+      },
+      next(): IteratorResult<number | null> {
+        const value = values[index++];
+        if (value === undefined) {
+          return { done: true, value: undefined };
+        }
+        return { done: false, value };
+      },
+      return(): IteratorResult<number | null> {
+        returned = true;
+        throw new Error('ignored return error');
+      },
+    };
+
+    expectJSON(await complete({ listField }, '[Int!]')).toDeepEqual({
+      data: { listField: null },
+      errors: [
+        {
+          message: 'Cannot return null for non-nullable field Query.listField.',
+          locations: [{ line: 1, column: 3 }],
+          path: ['listField', 1],
+        },
+      ],
+    });
+    expect(index).to.equal(2);
+    expect(returned).to.equal(true);
+  });
+});
+
 describe('Execute: Accepts async iterables as list value', () => {
   function complete(rootValue: unknown, as: string = '[String]') {
     return execute({

src/execution/execute.ts

@@ -1018,13 +1018,13 @@ function getStreamUsage(
  * Complete a async iterator value by completing the result and calling
  * recursively until all the results are completed.
  */
-async function completeAsyncIteratorValue(
+async function completeAsyncIterable(
   exeContext: ExecutionContext,
   itemType: GraphQLOutputType,
   fieldDetailsList: FieldDetailsList,
   info: GraphQLResolveInfo,
   path: Path,
-  asyncIterator: AsyncIterator<unknown>,
+  items: AsyncIterable<unknown>,
   incrementalContext: IncrementalContext | undefined,
   deferMap: ReadonlyMap<DeferUsage, DeferredFragmentRecord> | undefined,
 ): Promise<GraphQLWrappedResult<ReadonlyArray<unknown>>> {
@@ -1035,6 +1035,7 @@ async function completeAsyncIteratorValue(
     newDeferredFragmentRecords: undefined,
     incrementalDataRecords: undefined,
   };
+  const asyncIterator = items[Symbol.asyncIterator]();
   let index = 0;
   const streamUsage = getStreamUsage(
     exeContext.validatedExecutionArgs,
@@ -1131,10 +1132,7 @@ async function completeAsyncIteratorValue(
       index++;
     }
   } catch (error) {
-    asyncIterator.return?.().catch(() => {
-      /* c8 ignore next 1 */
-      // ignore error
-    });
+    returnIteratorIgnoringErrors(asyncIterator);
     throw error;
   }
 
@@ -1169,15 +1167,14 @@ function completeListValue(
     const maybeCancellableIterable = abortSignalListener
       ? cancellableIterable(result, abortSignalListener)
       : result;
-    const asyncIterator = maybeCancellableIterable[Symbol.asyncIterator]();
 
-    return completeAsyncIteratorValue(
+    return completeAsyncIterable(
       exeContext,
       itemType,
       fieldDetailsList,
       info,
       path,
-      asyncIterator,
+      maybeCancellableIterable,
       incrementalContext,
       deferMap,
     );
@@ -1211,8 +1208,6 @@ function completeIterableValue(
   incrementalContext: IncrementalContext | undefined,
   deferMap: ReadonlyMap<DeferUsage, DeferredFragmentRecord> | undefined,
 ): PromiseOrValue<GraphQLWrappedResult<ReadonlyArray<unknown>>> {
-  // This is specified as a simple map, however we're optimizing the path
-  // where the list contains no Promises by avoiding creating another Promise.
   let containsPromise = false;
   const completedResults: Array<unknown> = [];
   const graphqlWrappedResult: GraphQLWrappedResult<Array<unknown>> = {
@@ -1227,38 +1222,62 @@ function completeIterableValue(
     path,
   );
   const iterator = items[Symbol.iterator]();
-  let iteration = iterator.next();
-  while (!iteration.done) {
-    const item = iteration.value;
+  try {
+    while (true) {
+      if (streamUsage && index >= streamUsage.initialCount) {
+        const iteration = iterator.next();
+        if (!iteration.done) {
+          const item = iteration.value;
+          const syncStreamRecord: StreamRecord = {
+            label: streamUsage.label,
+            path,
+            streamItemQueue: buildSyncStreamItemQueue(
+              item,
+              index,
+              path,
+              iterator,
+              exeContext,
+              streamUsage.fieldDetailsList,
+              info,
+              itemType,
+            ),
+          };
 
-    if (streamUsage && index >= streamUsage.initialCount) {
-      const syncStreamRecord: StreamRecord = {
-        label: streamUsage.label,
-        path,
-        streamItemQueue: buildSyncStreamItemQueue(
-          item,
-          index,
-          path,
-          iterator,
-          exeContext,
-          streamUsage.fieldDetailsList,
-          info,
-          itemType,
-        ),
-      };
+          addIncrementalDataRecords(graphqlWrappedResult, [syncStreamRecord]);
+        }
+        break;
+      }
 
-      addIncrementalDataRecords(graphqlWrappedResult, [syncStreamRecord]);
-      break;
-    }
+      const iteration = iterator.next();
+      if (iteration.done) {
+        break;
+      }
 
-    // No need to modify the info object containing the path,
-    // since from here on it is not ever accessed by resolver functions.
-    const itemPath = addPath(path, index, undefined);
+      const item = iteration.value;
 
-    if (isPromise(item)) {
-      completedResults.push(
-        completePromisedListItemValue(
+      // No need to modify the info object containing the path,
+      // since from here on it is not ever accessed by resolver functions.
+      const itemPath = addPath(path, index, undefined);
+
+      if (isPromise(item)) {
+        completedResults.push(
+          completePromisedListItemValue(
+            item,
+            graphqlWrappedResult,
+            exeContext,
+            itemType,
+            fieldDetailsList,
+            info,
+            itemPath,
+            incrementalContext,
+            deferMap,
+          ),
+        );
+        containsPromise = true;
+      } else if (
+        completeListItemValue(
           item,
+          completedResults,
           graphqlWrappedResult,
           exeContext,
           itemType,
@@ -1267,28 +1286,15 @@ function completeIterableValue(
           itemPath,
           incrementalContext,
           deferMap,
-        ),
-      );
-      containsPromise = true;
-    } else if (
-      completeListItemValue(
-        item,
-        completedResults,
-        graphqlWrappedResult,
-        exeContext,
-        itemType,
-        fieldDetailsList,
-        info,
-        itemPath,
-        incrementalContext,
-        deferMap,
-      )
-    ) {
-      containsPromise = true;
+        )
+      ) {
+        containsPromise = true;
+      }
+      index++;
     }
-    index++;
-
-    iteration = iterator.next();
+  } catch (error) {
+    returnIteratorIgnoringErrors(iterator);
+    throw error;
   }
 
   return containsPromise
@@ -2442,3 +2448,18 @@ function buildStreamItemResult(
     incrementalDataRecords,
   };
 }
+
+function returnIteratorIgnoringErrors(
+  iterator: Iterator<unknown> | AsyncIterator<unknown>,
+) {
+  try {
+    const result = iterator.return?.();
+    if (isPromise(result)) {
+      result.catch(() => {
+        // ignore errors
+      });
+    }
+  } catch {
+    // ignore errors
+  }
+}