diff --git a/src/execution/Executor.ts b/src/execution/Executor.ts index 9a083e55f8..47af43a318 100644 --- a/src/execution/Executor.ts +++ b/src/execution/Executor.ts @@ -54,6 +54,7 @@ import { collectFields, collectSubfields as _collectSubfields, } from './collectFields.js'; +import { collectIteratorPromises } from './collectIteratorPromises.js'; import { buildResolveInfo } from './execute.js'; import type { StreamUsage } from './getStreamUsage.js'; import { getStreamUsage as _getStreamUsage } from './getStreamUsage.js'; @@ -833,6 +834,11 @@ export class Executor< } catch (error) { // eslint-disable-next-line @typescript-eslint/no-floating-promises returnIteratorCatchingErrors(asyncIterator); + if (containsPromise) { + return Promise.all(completedResults).finally(() => { + throw error; + }); + } throw error; } @@ -966,8 +972,13 @@ export class Executor< index++; } } catch (error) { - // eslint-disable-next-line @typescript-eslint/no-floating-promises - returnIteratorCatchingErrors(iterator); + const maybePromises = containsPromise ? completedResults : []; + maybePromises.push(...collectIteratorPromises(iterator)); + if (maybePromises.length) { + return Promise.all(maybePromises).finally(() => { + throw error; + }); + } throw error; } diff --git a/src/execution/__tests__/collectIteratorPromises-test.ts b/src/execution/__tests__/collectIteratorPromises-test.ts new file mode 100644 index 0000000000..0021f75e54 --- /dev/null +++ b/src/execution/__tests__/collectIteratorPromises-test.ts @@ -0,0 +1,41 @@ +import { expect } from 'chai'; +import { describe, it } from 'mocha'; + +import { collectIteratorPromises } from '../collectIteratorPromises.js'; + +describe('collectIteratorPromises', () => { + it('collects promise values until completion', () => { + const first = Promise.resolve(1); + const second = Promise.resolve(2); + const values: Array = [first, 'x', second]; + + const iterator: Iterator = { + next() { + const value = values.shift(); + if (value === undefined) { + return { done: true, value: undefined }; + } + return { done: false, value }; + }, + }; + + expect(collectIteratorPromises(iterator)).to.deep.equal([first, second]); + }); + + it('returns collected promises when draining throws', () => { + const first = Promise.resolve(1); + let nextCalls = 0; + + const iterator: Iterator = { + next() { + nextCalls += 1; + if (nextCalls === 1) { + return { done: false, value: first }; + } + throw new Error('bad'); + }, + }; + + expect(collectIteratorPromises(iterator)).to.deep.equal([first]); + }); +}); diff --git a/src/execution/__tests__/lists-test.ts b/src/execution/__tests__/lists-test.ts index 8605631216..117239b6f0 100644 --- a/src/execution/__tests__/lists-test.ts +++ b/src/execution/__tests__/lists-test.ts @@ -2,6 +2,7 @@ import { assert, expect } from 'chai'; import { describe, it } from 'mocha'; import { expectJSON } from '../../__testUtils__/expectJSON.js'; +import { resolveOnNextTick } from '../../__testUtils__/resolveOnNextTick.js'; import type { PromiseOrValue } from '../../jsutils/PromiseOrValue.js'; @@ -21,6 +22,14 @@ import { buildSchema } from '../../utilities/buildASTSchema.js'; import { execute, executeSync } from '../execute.js'; import type { ExecutionResult } from '../Executor.js'; +function delayedReject(message: string): Promise { + return (async () => { + await resolveOnNextTick(); + await resolveOnNextTick(); + throw new Error(message); + })(); +} + describe('Execute: Accepts any iterable as list value', () => { function complete(rootValue: unknown) { return executeSync({ @@ -77,13 +86,18 @@ describe('Execute: Accepts any iterable as list value', () => { }); }); - it('Ignores iterator return errors when iteration throws', () => { + it('Does not call iterator `return` when iteration throws', () => { let returnCalled = false; + let nextCalls = 0; const listField = { [Symbol.iterator]() { return { next() { - throw new Error('bad'); + nextCalls++; + if (nextCalls === 1) { + throw new Error('bad'); + } + return { done: true, value: undefined }; }, return() { returnCalled = true; @@ -103,7 +117,8 @@ describe('Execute: Accepts any iterable as list value', () => { }, ], }); - expect(returnCalled).to.equal(true); + expect(nextCalls).to.equal(2); + expect(returnCalled).to.equal(false); }); }); @@ -116,7 +131,7 @@ describe('Execute: Handles abrupt completion in synchronous iterables', () => { }); } - it('closes the iterator when `next` throws', async () => { + it('drains the iterator when `next` throws', async () => { let returned = false; let nextCalls = 0; @@ -129,7 +144,10 @@ describe('Execute: Handles abrupt completion in synchronous iterables', () => { if (nextCalls === 1) { return { done: false, value: 'ok' }; } - throw new Error('bad'); + if (nextCalls === 2) { + throw new Error('bad'); + } + return { done: true, value: undefined }; }, return(): IteratorResult { returned = true; @@ -147,11 +165,11 @@ describe('Execute: Handles abrupt completion in synchronous iterables', () => { }, ], }); - expect(nextCalls).to.equal(2); - expect(returned).to.equal(true); + expect(nextCalls).to.equal(3); + expect(returned).to.equal(false); }); - it('closes the iterator when a null bubbles up from a non-null item', async () => { + it('drains the iterator when a null bubbles up from a non-null item', async () => { const values = [1, null, 2]; let index = 0; let returned = false; @@ -183,34 +201,98 @@ describe('Execute: Handles abrupt completion in synchronous iterables', () => { }, ], }); - expect(index).to.equal(2); - expect(returned).to.equal(true); + expect(index).to.equal(4); + expect(returned).to.equal(false); }); - it('ignores errors thrown by the iterator `return` method', async () => { - const values = [1, null, 2]; - let index = 0; + it('handles iterator errors with later pending promises without calling `return`', async () => { + let unhandledRejection: unknown = null; + const unhandledRejectionListener = (reason: unknown) => { + unhandledRejection = reason; + }; + // eslint-disable-next-line no-undef + process.on('unhandledRejection', unhandledRejectionListener); let returned = false; + let nextCalls = 0; + const laterPromise = delayedReject('later bad'); - const listField: IterableIterator = { - [Symbol.iterator](): IterableIterator { + const listField: IterableIterator> = { + [Symbol.iterator](): IterableIterator> { return this; }, - next(): IteratorResult { + next(): IteratorResult> { + nextCalls++; + if (nextCalls === 1) { + return { done: false, value: 1 }; + } + if (nextCalls === 2) { + throw new Error('bad'); + } + if (nextCalls === 3) { + return { done: false, value: laterPromise }; + } + return { done: true, value: undefined }; + }, + return(): IteratorResult> { + returned = true; + throw new Error('ignored return error'); + }, + }; + + expectJSON(await complete({ listField })).toDeepEqual({ + data: { listField: null }, + errors: [ + { + message: 'bad', + locations: [{ line: 1, column: 3 }], + path: ['listField'], + }, + ], + }); + + await new Promise((resolve) => setTimeout(resolve, 20)); + + // eslint-disable-next-line no-undef + process.removeListener('unhandledRejection', unhandledRejectionListener); + + expect(nextCalls).to.equal(4); + expect(returned).to.equal(false); + expect(unhandledRejection).to.equal(null); + }); + + it('handles sync errors with later pending promises without calling `return`', async () => { + let unhandledRejection: unknown = null; + const unhandledRejectionListener = (reason: unknown) => { + unhandledRejection = reason; + }; + // eslint-disable-next-line no-undef + process.on('unhandledRejection', unhandledRejectionListener); + let returned = false; + let index = 0; + const values = [ + delayedReject('first bad'), + null, + delayedReject('third bad'), + ]; + const listField: IterableIterator | null> = { + [Symbol.iterator](): IterableIterator | null> { + return this; + }, + next(): IteratorResult | null> { const value = values[index++]; if (value === undefined) { return { done: true, value: undefined }; } return { done: false, value }; }, - return(): IteratorResult { + return(): IteratorResult | null> { returned = true; throw new Error('ignored return error'); }, }; - expectJSON(await complete({ listField }, '[Int!]')).toDeepEqual({ - data: { listField: null }, + expectJSON(await complete({ listField }, '[String!]!')).toDeepEqual({ + data: null, errors: [ { message: 'Cannot return null for non-nullable field Query.listField.', @@ -219,8 +301,15 @@ describe('Execute: Handles abrupt completion in synchronous iterables', () => { }, ], }); - expect(index).to.equal(2); - expect(returned).to.equal(true); + + await new Promise((resolve) => setTimeout(resolve, 20)); + + // eslint-disable-next-line no-undef + process.removeListener('unhandledRejection', unhandledRejectionListener); + + expect(returned).to.equal(false); + expect(index).to.equal(4); + expect(unhandledRejection).to.equal(null); }); }); @@ -235,7 +324,17 @@ describe('Execute: Accepts async iterables as list value', () => { function completeObjectList( resolve: GraphQLFieldResolver<{ index: number }, unknown>, + nonNullable = false, ): PromiseOrValue { + const ObjectWrapperType = new GraphQLObjectType({ + name: 'ObjectWrapper', + fields: { + index: { + type: new GraphQLNonNull(GraphQLString), + resolve, + }, + }, + }); const schema = new GraphQLSchema({ query: new GraphQLObjectType({ name: 'Query', @@ -247,15 +346,9 @@ describe('Execute: Accepts async iterables as list value', () => { yield await Promise.resolve({ index: 2 }); }, type: new GraphQLList( - new GraphQLObjectType({ - name: 'ObjectWrapper', - fields: { - index: { - type: new GraphQLNonNull(GraphQLString), - resolve, - }, - }, - }), + nonNullable + ? new GraphQLNonNull(ObjectWrapperType) + : ObjectWrapperType, ), }, }, @@ -362,6 +455,41 @@ describe('Execute: Accepts async iterables as list value', () => { ], }); }); + + it('handles mixture of sync and async errors in AsyncIterables', async () => { + let unhandledRejection: unknown = null; + const unhandledRejectionListener = (reason: unknown) => { + unhandledRejection = reason; + }; + // eslint-disable-next-line no-undef + process.on('unhandledRejection', unhandledRejectionListener); + + expectJSON( + await completeObjectList(({ index }) => { + if (index === 0) { + return delayedReject('bad'); + } + throw new Error('also bad'); + }, true), + ).toDeepEqual({ + data: { listField: null }, + errors: [ + { + message: 'also bad', + locations: [{ line: 1, column: 15 }], + path: ['listField', 1, 'index'], + }, + ], + }); + + await new Promise((resolve) => setTimeout(resolve, 20)); + + // eslint-disable-next-line no-undef + process.removeListener('unhandledRejection', unhandledRejectionListener); + + expect(unhandledRejection).to.equal(null); + }); + it('Handles nulls yielded by async generator', async () => { async function* listField() { yield await Promise.resolve(1); diff --git a/src/execution/collectIteratorPromises.ts b/src/execution/collectIteratorPromises.ts new file mode 100644 index 0000000000..af44b158b8 --- /dev/null +++ b/src/execution/collectIteratorPromises.ts @@ -0,0 +1,25 @@ +import { isPromise } from '../jsutils/isPromise.js'; + +/** + * Drain a sync iterator after abrupt completion so later promise rejections + * can be observed before they become unhandled. + */ +export function collectIteratorPromises( + iterator: Iterator, +): Array> { + const promises = []; + try { + while (true) { + const iteration = iterator.next(); + if (iteration.done) { + return promises; + } + if (isPromise(iteration.value)) { + promises.push(iteration.value); + } + } + } catch { + // Ignore errors while draining the remaining items. + return promises; + } +} diff --git a/src/execution/incremental/IncrementalExecutor.ts b/src/execution/incremental/IncrementalExecutor.ts index b1d515a52c..5563a7bc13 100644 --- a/src/execution/incremental/IncrementalExecutor.ts +++ b/src/execution/incremental/IncrementalExecutor.ts @@ -29,6 +29,7 @@ import type { GroupedFieldSet, } from '../collectFields.js'; import { collectSubfields as _collectSubfields } from '../collectFields.js'; +import { collectIteratorPromises } from '../collectIteratorPromises.js'; import type { ExecutionResult, FormattedExecutionResult, @@ -745,9 +746,15 @@ export class IncrementalExecutor< abortPromises.push(result); } } - const returned = returnIteratorCatchingErrors(iterator); - if (isPromise(returned)) { + if (isAsync) { + const returned = returnIteratorCatchingErrors( + iterator as AsyncIterator, + ); abortPromises.push(returned); + } else { + abortPromises.push( + ...collectIteratorPromises(iterator as Iterator), + ); } if (abortPromises.length > 0) { return Promise.allSettled(abortPromises).then(() => undefined); diff --git a/src/execution/incremental/__tests__/stream-test.ts b/src/execution/incremental/__tests__/stream-test.ts index cfa69e7dcb..da2012cee4 100644 --- a/src/execution/incremental/__tests__/stream-test.ts +++ b/src/execution/incremental/__tests__/stream-test.ts @@ -43,6 +43,14 @@ const friends = [ { name: 'Leia', id: 3 }, ]; +function delayedReject(message: string): Promise { + return (async () => { + await resolveOnNextTick(); + await resolveOnNextTick(); + throw new Error(message); + })(); +} + const query = new GraphQLObjectType({ fields: { scalarList: { @@ -208,6 +216,46 @@ describe('Execute: stream directive', () => { }, ]); }); + it('Does not call `return` on an exhausted sync iterator', async () => { + const document = parse('{ scalarList @stream(initialCount: 1) }'); + let index = 0; + let returned = false; + const values = ['apple', 'banana', 'coconut']; + const result = await complete(document, { + scalarList: { + [Symbol.iterator]() { + return this; + }, + next() { + const value = values[index++]; + if (value === undefined) { + return { done: true, value: undefined }; + } + return { done: false, value }; + }, + return() { + returned = true; + throw new Error('ignored return error'); + }, + }, + }); + expectJSON(result).toDeepEqual([ + { + data: { + scalarList: ['apple'], + }, + pending: [{ id: '0', path: ['scalarList'] }], + hasNext: true, + }, + { + incremental: [{ items: ['banana', 'coconut'], id: '0' }], + completed: [{ id: '0' }], + hasNext: false, + }, + ]); + expect(returned).to.equal(false); + expect(index).to.equal(4); + }); it('Can use default value of initialCount', async () => { const document = parse('{ scalarList @stream }'); const result = await complete(document, { @@ -1172,6 +1220,77 @@ describe('Execute: stream directive', () => { }, ]); }); + it('Drains sync iterators with later promises when null bubbles past the stream', async () => { + const document = parse(` + query { + nonNullFriendList @stream(initialCount: 1) { + name + } + } + `); + let unhandledRejection: unknown = null; + const unhandledRejectionListener = (reason: unknown) => { + unhandledRejection = reason; + }; + // eslint-disable-next-line no-undef + process.on('unhandledRejection', unhandledRejectionListener); + let index = 0; + let returned = false; + const values = [friends[0], null, delayedReject('third bad')]; + + const result = await complete(document, { + nonNullFriendList: { + [Symbol.iterator]() { + return this; + }, + next() { + const value = values[index++]; + if (value === undefined) { + return { done: true, value: undefined }; + } + return { done: false, value }; + }, + return() { + returned = true; + throw new Error('ignored return error'); + }, + }, + }); + + await new Promise((resolve) => setTimeout(resolve, 20)); + + // eslint-disable-next-line no-undef + process.removeListener('unhandledRejection', unhandledRejectionListener); + + expectJSON(result).toDeepEqual([ + { + data: { + nonNullFriendList: [{ name: 'Luke' }], + }, + pending: [{ id: '0', path: ['nonNullFriendList'] }], + hasNext: true, + }, + { + completed: [ + { + id: '0', + errors: [ + { + message: + 'Cannot return null for non-nullable field Query.nonNullFriendList.', + locations: [{ line: 3, column: 9 }], + path: ['nonNullFriendList', 1], + }, + ], + }, + ], + hasNext: false, + }, + ]); + expect(returned).to.equal(false); + expect(index).to.equal(4); + expect(unhandledRejection).to.equal(null); + }); it('Handles errors thrown by completeValue after initialCount is reached', async () => { const document = parse(` query { @@ -3367,6 +3486,7 @@ describe('Execute: stream directive (cancellation)', () => { promiseWithResolvers(); let count = 0; let done = false; + let returned = false; const iterator = { [Symbol.iterator]() { return this; @@ -3384,6 +3504,10 @@ describe('Execute: stream directive (cancellation)', () => { } return { value: String(count), done: false }; }, + return() { + returned = true; + throw new Error('ignored return error'); + }, }; const result = await experimentalExecuteIncrementally({ @@ -3400,6 +3524,7 @@ describe('Execute: stream directive (cancellation)', () => { await resolveOnNextTick(); const stream = result.subsequentResults[Symbol.asyncIterator](); await expectPromise(stream.return()).toResolve(); + expect(returned).to.equal(false); }); it('cancels tasks and streams when aborted before initial execution finishes', async () => { diff --git a/src/execution/returnIteratorCatchingErrors.ts b/src/execution/returnIteratorCatchingErrors.ts index df53d79bd0..6b62d4c063 100644 --- a/src/execution/returnIteratorCatchingErrors.ts +++ b/src/execution/returnIteratorCatchingErrors.ts @@ -1,17 +1,8 @@ -import { isPromise } from '../jsutils/isPromise.js'; -import type { PromiseOrValue } from '../jsutils/PromiseOrValue.js'; - -export function returnIteratorCatchingErrors( - iterator: Iterator | AsyncIterator, -): PromiseOrValue { +export async function returnIteratorCatchingErrors( + iterator: AsyncIterator, +): Promise { try { - const result = iterator.return?.(); - if (isPromise(result)) { - return result.then( - () => undefined, - () => undefined, - ); - } + await iterator.return?.(); } catch /* c8 ignore next 2 */ { // ignore errors }