Skip to content

fix(stream): continue on iterator error values #2980

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions benchmark/benchmark.js
Original file line number Diff line number Diff line change
Expand Up @@ -346,9 +346,9 @@ function sampleModule(modulePath) {

clock(7, module.measure); // warm up
global.gc();
process.nextTick(() => {
process.nextTick(async () => {
const memBaseline = process.memoryUsage().heapUsed;
const clocked = clock(module.count, module.measure);
const clocked = await clock(module.count, module.measure);
process.send({
name: module.name,
clocked: clocked / module.count,
Expand All @@ -357,10 +357,10 @@ function sampleModule(modulePath) {
});

// Clocks the time taken to execute a test per cycle (secs).
function clock(count, fn) {
async function clock(count, fn) {
const start = process.hrtime.bigint();
for (let i = 0; i < count; ++i) {
fn();
await fn();
}
return Number(process.hrtime.bigint() - start);
}
Expand Down
28 changes: 28 additions & 0 deletions benchmark/list-async-benchmark.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
'use strict';

const { parse } = require('graphql/language/parser.js');
const { execute } = require('graphql/execution/execute.js');
const { buildSchema } = require('graphql/utilities/buildASTSchema.js');

const schema = buildSchema('type Query { listField: [String] }');
const document = parse('{ listField }');

function listField() {
const results = [];
for (let index = 0; index < 100000; index++) {
results.push(Promise.resolve(index));
}
return results;
}

module.exports = {
name: 'Execute Asynchronous List Field',
count: 10,
async measure() {
await execute({
schema,
document,
rootValue: { listField },
});
},
};
26 changes: 26 additions & 0 deletions benchmark/list-asyncIterable-benchmark.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
'use strict';

const { parse } = require('graphql/language/parser.js');
const { execute } = require('graphql/execution/execute.js');
const { buildSchema } = require('graphql/utilities/buildASTSchema.js');

const schema = buildSchema('type Query { listField: [String] }');
const document = parse('{ listField }');

async function* listField() {
for (let index = 0; index < 100000; index++) {
yield index;
}
}

module.exports = {
name: 'Execute Async Iterable List Field',
count: 10,
async measure() {
await execute({
schema,
document,
rootValue: { listField },
});
},
};
28 changes: 28 additions & 0 deletions benchmark/list-sync-benchmark.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
'use strict';

const { parse } = require('graphql/language/parser.js');
const { execute } = require('graphql/execution/execute.js');
const { buildSchema } = require('graphql/utilities/buildASTSchema.js');

const schema = buildSchema('type Query { listField: [String] }');
const document = parse('{ listField }');

function listField() {
const results = [];
for (let index = 0; index < 100000; index++) {
results.push(index);
}
return results;
}

module.exports = {
name: 'Execute Synchronous List Field',
count: 10,
async measure() {
await execute({
schema,
document,
rootValue: { listField },
});
},
};
141 changes: 141 additions & 0 deletions src/execution/__tests__/lists-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@ import { expect } from 'chai';
import { describe, it } from 'mocha';

import { parse } from '../../language/parser';
import { GraphQLList, GraphQLObjectType } from '../../type/definition';
import { GraphQLString } from '../../type/scalars';
import { GraphQLSchema } from '../../type/schema';

import { buildSchema } from '../../utilities/buildASTSchema';

Expand Down Expand Up @@ -64,6 +67,144 @@ describe('Execute: Accepts any iterable as list value', () => {
});
});

describe('Execute: Accepts async iterables as list value', () => {
function complete(rootValue: mixed) {
return execute({
schema: buildSchema('type Query { listField: [String] }'),
document: parse('{ listField }'),
rootValue,
});
}

function completeObjectList(resolve) {
const schema = new GraphQLSchema({
query: new GraphQLObjectType({
name: 'Query',
fields: {
listField: {
resolve: async function* listField() {
yield await { index: 0 };
yield await { index: 1 };
yield await { index: 2 };
},
type: new GraphQLList(
new GraphQLObjectType({
name: 'ObjectWrapper',
fields: {
index: {
type: GraphQLString,
resolve,
},
},
}),
),
},
},
}),
});
return execute({
schema,
document: parse('{ listField { index } }'),
});
}

it('Accepts an AsyncGenerator function as a List value', async () => {
async function* listField() {
yield await 'two';
yield await 4;
yield await false;
}

expect(await complete({ listField })).to.deep.equal({
data: { listField: ['two', '4', 'false'] },
});
});

it('Handles an AsyncGenerator function that throws', async () => {
async function* listField() {
yield await 'two';
yield await 4;
throw new Error('bad');
}

expect(await complete({ listField })).to.deep.equal({
data: { listField: ['two', '4', null] },
errors: [
{
message: 'bad',
locations: [{ line: 1, column: 3 }],
path: ['listField', 2],
},
],
});
});

it('Handles an AsyncGenerator function where an intermediate value triggers an error', async () => {
async function* listField() {
yield await 'two';
yield await {};
yield await 4;
}

expect(await complete({ listField })).to.deep.equal({
data: { listField: ['two', null, '4'] },
errors: [
{
message: 'String cannot represent value: {}',
locations: [{ line: 1, column: 3 }],
path: ['listField', 1],
},
],
});
});

it('Handles errors from `completeValue` in AsyncIterables', async () => {
async function* listField() {
yield await 'two';
yield await {};
}

expect(await complete({ listField })).to.deep.equal({
data: { listField: ['two', null] },
errors: [
{
message: 'String cannot represent value: {}',
locations: [{ line: 1, column: 3 }],
path: ['listField', 1],
},
],
});
});

it('Handles promises from `completeValue` in AsyncIterables', async () => {
expect(
await completeObjectList(({ index }) => Promise.resolve(index)),
).to.deep.equal({
data: { listField: [{ index: '0' }, { index: '1' }, { index: '2' }] },
});
});

it('Handles rejected promises from `completeValue` in AsyncIterables', async () => {
expect(
await completeObjectList(({ index }) => {
if (index === 2) {
return Promise.reject(new Error('bad'));
}
return Promise.resolve(index);
}),
).to.deep.equal({
data: { listField: [{ index: '0' }, { index: '1' }, { index: null }] },
errors: [
{
message: 'bad',
locations: [{ line: 1, column: 15 }],
path: ['listField', 2, 'index'],
},
],
});
});
});

describe('Execute: Handles list nullability', () => {
async function complete(args: {| listField: mixed, as: string |}) {
const { listField, as } = args;
Expand Down
84 changes: 83 additions & 1 deletion src/execution/execute.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import arrayFrom from '../polyfills/arrayFrom';
import { SYMBOL_ASYNC_ITERATOR } from '../polyfills/symbols';

import type { Path } from '../jsutils/Path';
import type { ObjMap } from '../jsutils/ObjMap';
Expand All @@ -8,6 +9,7 @@ import memoize3 from '../jsutils/memoize3';
import invariant from '../jsutils/invariant';
import devAssert from '../jsutils/devAssert';
import isPromise from '../jsutils/isPromise';
import isAsyncIterable from '../jsutils/isAsyncIterable';
import isObjectLike from '../jsutils/isObjectLike';
import isCollection from '../jsutils/isCollection';
import promiseReduce from '../jsutils/promiseReduce';
Expand Down Expand Up @@ -855,6 +857,72 @@ function completeValue(
);
}

/**
* Complete a async iterator value by completing the result and calling
* recursively until all the results are completed.
*/
function completeAsyncIteratorValue(
exeContext: ExecutionContext,
itemType: GraphQLOutputType,
fieldNodes: $ReadOnlyArray<FieldNode>,
info: GraphQLResolveInfo,
path: Path,
iterator: AsyncIterator<mixed>,
): Promise<$ReadOnlyArray<mixed>> {
let containsPromise = false;
return new Promise((resolve) => {
function next(index, completedResults) {
const fieldPath = addPath(path, index, undefined);
iterator.next().then(
({ value, done }) => {
if (done) {
resolve(completedResults);
return;
}
// TODO can the error checking logic be consolidated with completeListValue?
try {
const completedItem = completeValue(
exeContext,
itemType,
fieldNodes,
info,
fieldPath,
value,
);
if (isPromise(completedItem)) {
containsPromise = true;
}
completedResults.push(completedItem);
} catch (rawError) {
completedResults.push(null);
const error = locatedError(
rawError,
fieldNodes,
pathToArray(fieldPath),
);
handleFieldError(error, itemType, exeContext);
}

next(index + 1, completedResults);
},
(rawError) => {
completedResults.push(null);
const error = locatedError(
rawError,
fieldNodes,
pathToArray(fieldPath),
);
handleFieldError(error, itemType, exeContext);
resolve(completedResults);
},
);
}
next(0, []);
}).then((completedResults) =>
containsPromise ? Promise.all(completedResults) : completedResults,
);
}

/**
* Complete a list value by completing each item in the list with the
* inner type
Expand All @@ -867,6 +935,21 @@ function completeListValue(
path: Path,
result: mixed,
): PromiseOrValue<$ReadOnlyArray<mixed>> {
const itemType = returnType.ofType;

if (isAsyncIterable(result)) {
const iterator = result[SYMBOL_ASYNC_ITERATOR]();

return completeAsyncIteratorValue(
exeContext,
itemType,
fieldNodes,
info,
path,
iterator,
);
}

if (!isCollection(result)) {
throw new GraphQLError(
`Expected Iterable, but did not find one for field "${info.parentType.name}.${info.fieldName}".`,
Expand All @@ -875,7 +958,6 @@ function completeListValue(

// This is specified as a simple map, however we're optimizing the path
// where the list contains no Promises by avoiding creating another Promise.
const itemType = returnType.ofType;
let containsPromise = false;
const completedResults = arrayFrom(result, (item, index) => {
// No need to modify the info object containing the path,
Expand Down